--===============4198909889830797956== Content-Type: multipart/alternative; boundary=90e6ba475dcf2759ba051c0bfa39 --90e6ba475dcf2759ba051c0bfa39 Content-Type: text/plain; charset=UTF-8 On Wed, Jul 29, 2015 at 2:55 PM, Richard Trieu wrote: > On Wed, Jul 29, 2015 at 1:52 PM, Richard Smith > wrote: > >> On Tue, Jul 28, 2015 at 12:06 PM, Richard Trieu >> wrote: >> >>> Author: rtrieu >>> Date: Tue Jul 28 14:06:16 2015 >>> New Revision: 243463 >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=243463&view=rev >>> Log: >>> Do not give a -Wredundant-move warning when removing the move will >>> result in an >>> error. >>> >>> If the object being moved has a move constructor and a deleted copy >>> constructor, >>> std::move is required, otherwise Clang will give a deleted constructor >>> error. >>> >>> Modified: >>> cfe/trunk/lib/Sema/SemaInit.cpp >>> cfe/trunk/test/SemaCXX/warn-redundant-move.cpp >>> >>> Modified: cfe/trunk/lib/Sema/SemaInit.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=243463&r1=243462&r2=243463&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/lib/Sema/SemaInit.cpp (original) >>> +++ cfe/trunk/lib/Sema/SemaInit.cpp Tue Jul 28 14:06:16 2015 >>> @@ -5983,9 +5983,19 @@ static void CheckMoveOnConstruction(Sema >>> if (!VD || !VD->hasLocalStorage()) >>> return; >>> >>> - if (!VD->getType()->isRecordType()) >>> + QualType SourceType = VD->getType(); >>> + if (!SourceType->isRecordType()) >>> return; >>> >>> + if (!S.Context.hasSameUnqualifiedType(DestType, SourceType)) { >>> + if (CXXRecordDecl *RD = SourceType->getAsCXXRecordDecl()) { >>> + for (auto* Construct : RD->ctors()) { >>> + if (Construct->isCopyConstructor() && Construct->isDeleted()) >>> + return; >>> >> >> This is not the right change, for a couple of reasons. In particular: >> 1) the constructor that would be selected might not be the copy >> constructor, so you're not checking the right thing >> 2) the purpose of the warning is to warn on cases where you'd get an >> implicit move even without the std::move call, and you seem to now be >> looking for cases where the call to std::move would result in a copy >> >> Until we have an implementation of DR1579, the best thing to do is >> probably just to disable/remove the -Wredundant-move warning. As far as I >> recall, its only purpose was to warn on the cases where DR1579 applies, and >> there are no such cases since we don't implement DR1579. >> > > Wouldn't the case of returning a function parameter still be valid for > -Wredundant-move? We should keep that for Clang and the remove the rest of > the redundant move warning. > Ah yes, you're right, we should keep the -Wredundant-move warning for that case. > + } >>> + } >>> + } >>> + >>> // If we're returning a function parameter, copy elision >>> // is not possible. >>> if (isa(VD)) >>> >>> Modified: cfe/trunk/test/SemaCXX/warn-redundant-move.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-redundant-move.cpp?rev=243463&r1=243462&r2=243463&view=diff >>> >>> >>> ============================================================================== >>> --- cfe/trunk/test/SemaCXX/warn-redundant-move.cpp (original) >>> +++ cfe/trunk/test/SemaCXX/warn-redundant-move.cpp Tue Jul 28 14:06:16 >>> 2015 >>> @@ -1,5 +1,5 @@ >>> // RUN: %clang_cc1 -fsyntax-only -Wredundant-move -std=c++11 -verify %s >>> -// RUN: %clang_cc1 -fsyntax-only -Wredundant-move -std=c++11 >>> -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s >>> +// RUN: not %clang_cc1 -fsyntax-only -Wredundant-move -std=c++11 >>> -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s >>> >>> // definitions for std::move >>> namespace std { >>> @@ -102,3 +102,39 @@ D test5(D d) { >>> // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:20}:"" >>> // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:21-[[@LINE-4]]:22}:"" >>> } >>> + >>> +// No more fix-its past here. >>> +// CHECK-NOT: fix-it >>> + >>> +// A deleted copy constructor will prevent moves without std::move >>> +struct E { >>> + E(E &&e); >>> + E(const E &e) = delete; >>> + // expected-note@-1{{deleted here}} >>> +}; >>> + >>> +struct F { >>> + F(E); >>> + // expected-note@-1{{passing argument to parameter here}} >>> +}; >>> + >>> +F test6(E e) { >>> + return e; >>> + // expected-error@-1{{call to deleted constructor of 'E'}} >>> + return std::move(e); >>> +} >>> + >>> +struct G { >>> + G(G &&g); >>> + // expected-note@-1{{copy constructor is implicitly deleted because >>> 'G' has a user-declared move constructor}} >>> +}; >>> + >>> +struct H { >>> + H(G); >>> + // expected-note@-1{{passing argument to parameter here}} >>> +}; >>> + >>> +H test6(G g) { >>> + return g; // expected-error{{call to implicitly-deleted copy >>> constructor of 'G'}} >>> + return std::move(g); >>> +} >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> cfe-commits@cs.uiuc.edu >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> >> >> > --90e6ba475dcf2759ba051c0bfa39 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: Quoted-printable
On W= ed, Jul 29, 2015 at 2:55 PM, Richard Trieu <rtrieu@google.com> wrote:
On Wed, Jul = 29, 2015 at 1:52 PM, Richard Smith <richard@metafoo.co.uk> wrote:
On Tue, Jul 28, 2015 at 12:06= PM, Richard Trieu <rtrieu@google.com> wrote:
Author: rtrieu
Date: Tue Jul 28 14:06:16 2015
New Revision: 243463

URL: http://llvm.org/viewv= c/llvm-project?rev=3D243463&view=3Drev
Log:
Do not give a -Wredundant-move warning when removing the move will result i= n an
error.

If the object being moved has a move constructor and a deleted copy constru= ctor,
std::move is required, otherwise Clang will give a deleted constructor erro= r.

Modified:
=C2=A0 =C2=A0 cfe/trunk/lib/Sema/SemaInit.cpp
=C2=A0 =C2=A0 cfe/trunk/test/SemaCXX/warn-redundant-move.cpp

Modified: cfe/trunk/lib/Sema/SemaInit.cpp
URL: http://llvm.org/viewvc/llvm-proje= ct/cfe/trunk/lib/Sema/SemaInit.cpp?rev=3D243463&r1=3D243462&r2=3D24= 3463&view=3Ddiff
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D
--- cfe/trunk/lib/Sema/SemaInit.cpp (original)
+++ cfe/trunk/lib/Sema/SemaInit.cpp Tue Jul 28 14:06:16 2015
@@ -5983,9 +5983,19 @@ static void CheckMoveOnConstruction(Sema
=C2=A0 =C2=A0 =C2=A0if (!VD || !VD->hasLocalStorage())
=C2=A0 =C2=A0 =C2=A0 =C2=A0return;

-=C2=A0 =C2=A0 if (!VD->getType()->isRecordType())
+=C2=A0 =C2=A0 QualType SourceType =3D VD->getType();
+=C2=A0 =C2=A0 if (!SourceType->isRecordType())
=C2=A0 =C2=A0 =C2=A0 =C2=A0return;

+=C2=A0 =C2=A0 if (!S.Context.hasSameUnqualifiedType(DestType, SourceType))= {
+=C2=A0 =C2=A0 =C2=A0 if (CXXRecordDecl *RD =3D SourceType->getAsCXXReco= rdDecl()) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 for (auto* Construct : RD->ctors()) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (Construct->isCopyConstructor() &= amp;& Construct->isDeleted())
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return;
<= div>
This is not the right change, for a couple of reasons. I= n particular:
=C2=A01) the constructor that would be selected mig= ht not be the copy constructor, so you're not checking the right thing<= /div>
=C2=A02) the purpose of the warning is to warn on cases where you= 'd get an implicit move even without the std::move call, and you seem t= o now be looking for cases where the call to std::move would result in a co= py

Until we have an implementation of DR1579, the = best thing to do is probably just to disable/remove the -Wredundant-move wa= rning. As far as I recall, its only purpose was to warn on the cases where = DR1579 applies, and there are no such cases since we don't implement DR= 1579.

W= ouldn't the case of returning a function parameter still be valid for -= Wredundant-move?=C2=A0 We should keep that for Clang and the remove the res= t of the redundant move warning.
<= br>
Ah yes, you're right, we should keep the -Wredundant-move= warning for that case.
=C2=A0=
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
+=C2=A0 =C2=A0 =C2=A0 }
+=C2=A0 =C2=A0 }
+
=C2=A0 =C2=A0 =C2=A0// If we're returning a function parameter, copy el= ision
=C2=A0 =C2=A0 =C2=A0// is not possible.
=C2=A0 =C2=A0 =C2=A0if (isa<ParmVarDecl>(VD))

Modified: cfe/trunk/test/SemaCXX/warn-redundant-move.cpp
URL: http://llvm.org/= viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-redundant-move.cpp?rev=3D24= 3463&r1=3D243462&r2=3D243463&view=3Ddiff

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D
--- cfe/trunk/test/SemaCXX/warn-redundant-move.cpp (original)
+++ cfe/trunk/test/SemaCXX/warn-redundant-move.cpp Tue Jul 28 14:06:16 2015=
@@ -1,5 +1,5 @@
=C2=A0// RUN: %clang_cc1 -fsyntax-only -Wredundant-move -std=3Dc++11 -verif= y %s
-// RUN: %clang_cc1 -fsyntax-only -Wredundant-move -std=3Dc++11 -fdiagnosti= cs-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: not %clang_cc1 -fsyntax-only -Wredundant-move -std=3Dc++11 -fdiagn= ostics-parseable-fixits %s 2>&1 | FileCheck %s

=C2=A0// definitions for std::move
=C2=A0namespace std {
@@ -102,3 +102,39 @@ D test5(D d) {
=C2=A0 =C2=A0// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]= ]:20}:""
=C2=A0 =C2=A0// CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:21-[[@LINE-4]= ]:22}:""
=C2=A0}
+
+// No more fix-its past here.
+// CHECK-NOT: fix-it
+
+// A deleted copy constructor will prevent moves without std::move
+struct E {
+=C2=A0 E(E &&e);
+=C2=A0 E(const E &e) =3D delete;
+=C2=A0 // expected-note@-1{{deleted here}}
+};
+
+struct F {
+=C2=A0 F(E);
+=C2=A0 // expected-note@-1{{passing argument to parameter here}}
+};
+
+F test6(E e) {
+=C2=A0 return e;
+=C2=A0 // expected-error@-1{{call to deleted constructor of 'E'}}<= br> +=C2=A0 return std::move(e);
+}
+
+struct G {
+=C2=A0 G(G &&g);
+=C2=A0 // expected-note@-1{{copy constructor is implicitly deleted because= 'G' has a user-declared move constructor}}
+};
+
+struct H {
+=C2=A0 H(G);
+=C2=A0 // expected-note@-1{{passing argument to parameter here}}
+};
+
+H test6(G g) {
+=C2=A0 return g;=C2=A0 // expected-error{{call to implicitly-deleted copy = constructor of 'G'}}
+=C2=A0 return std::move(g);
+}


_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-c= ommits



--90e6ba475dcf2759ba051c0bfa39-- --===============4198909889830797956== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits --===============4198909889830797956==--