[prev in list] [next in list] [prev in thread] [next in thread]
List: cfe-commits
Subject: Re: r243463 - Do not give a -Wredundant-move warning when removing the move will result in an
From: Richard Trieu <rtrieu () google ! com>
Date: 2015-07-29 21:55:22
Message-ID: CAPUYKWm3tUqzjS8bXZWGXnXSvO0HCcHET=n1sjDuN+_e=Wa3GQ () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
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/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.
>
>
> > + }
> > + }
> > + }
> > +
> > // If we're returning a function parameter, copy elision
> > // is not possible.
> > if (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=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
> >
>
>
[Attachment #5 (text/html)]
<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jul \
29, 2015 at 1:52 PM, Richard Smith <span dir="ltr"><<a \
href="mailto:richard@metafoo.co.uk" \
target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div \
class="gmail_quote"><span class="">On Tue, Jul 28, 2015 at 12:06 PM, Richard Trieu \
<span dir="ltr"><<a href="mailto:rtrieu@google.com" \
target="_blank">rtrieu@google.com</a>></span> wrote:<br></span><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><span class="">Author: rtrieu<br>
Date: Tue Jul 28 14:06:16 2015<br>
New Revision: 243463<br>
<br></span>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm \
-2Dproject-3Frev-3D243463-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW \
_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=1rzxhCNBgk8r4bCuMVDbORj8CL3yJ4B2v-R_ZJ5soHg&s=UrZReQf4jRMYJHhD0EbRy5GIZwoQGM8GcKC1lsSogvg&e=" \
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=243463&view=rev</a><span \
class=""><br> Log:<br>
Do not give a -Wredundant-move warning when removing the move will result in an<br>
error.<br>
<br>
If the object being moved has a move constructor and a deleted copy constructor,<br>
std::move is required, otherwise Clang will give a deleted constructor error.<br>
<br>
Modified:<br>
cfe/trunk/lib/Sema/SemaInit.cpp<br>
cfe/trunk/test/SemaCXX/warn-redundant-move.cpp<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaInit.cpp<br></span>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm \
-2Dproject_cfe_trunk_lib_Sema_SemaInit.cpp-3Frev-3D243463-26r1-3D243462-26r2-3D243463- \
26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnRE \
eq0cCxk&m=1rzxhCNBgk8r4bCuMVDbORj8CL3yJ4B2v-R_ZJ5soHg&s=o5Ff94b2NnLMr1hhy2ySsOzomxlQXa7MFQz1r1y0V0Y&e=" \
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=243463&r1=243462&r2=243463&view=diff</a><span \
class=""><br> ==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaInit.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaInit.cpp Tue Jul 28 14:06:16 2015<br>
@@ -5983,9 +5983,19 @@ static void CheckMoveOnConstruction(Sema<br>
if (!VD || !VD->hasLocalStorage())<br>
return;<br>
<br>
- if (!VD->getType()->isRecordType())<br>
+ QualType SourceType = VD->getType();<br>
+ if (!SourceType->isRecordType())<br>
return;<br>
<br>
+ if (!S.Context.hasSameUnqualifiedType(DestType, SourceType)) {<br>
+ if (CXXRecordDecl *RD = SourceType->getAsCXXRecordDecl()) {<br>
+ for (auto* Construct : RD->ctors()) {<br>
+ if (Construct->isCopyConstructor() && \
Construct->isDeleted())<br> + \
return;<br></span></blockquote><div><br></div><div>This is not the right change, for \
a couple of reasons. In particular:</div><div> 1) the constructor that would be \
selected might not be the copy constructor, so you're not checking the right \
thing</div><div> 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</div><div><br></div><div>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.</div></div></div></div></blockquote><div><br></div><div>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.</div><div> \
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div \
class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""> + }<br>
+ }<br>
+ }<br>
+<br>
// If we're returning a function parameter, copy elision<br>
// is not possible.<br>
if (isa<ParmVarDecl>(VD))<br>
<br>
Modified: cfe/trunk/test/SemaCXX/warn-redundant-move.cpp<br></span>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm \
-2Dproject_cfe_trunk_test_SemaCXX_warn-2Dredundant-2Dmove.cpp-3Frev-3D243463-26r1-3D24 \
3462-26r2-3D243463-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8Sy \
ngJ70KdZISM_ASROnREeq0cCxk&m=1rzxhCNBgk8r4bCuMVDbORj8CL3yJ4B2v-R_ZJ5soHg&s=ChDDuzyllauxOl1nPf3imFVp-vANHvQ6Y_YKnhd0p88&e=" \
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Se \
maCXX/warn-redundant-move.cpp?rev=243463&r1=243462&r2=243463&view=diff</a><div><div \
class="h5"><br> ==============================================================================<br>
--- cfe/trunk/test/SemaCXX/warn-redundant-move.cpp (original)<br>
+++ cfe/trunk/test/SemaCXX/warn-redundant-move.cpp Tue Jul 28 14:06:16 2015<br>
@@ -1,5 +1,5 @@<br>
// RUN: %clang_cc1 -fsyntax-only -Wredundant-move -std=c++11 -verify %s<br>
-// RUN: %clang_cc1 -fsyntax-only -Wredundant-move -std=c++11 \
-fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s<br> +// RUN: not \
%clang_cc1 -fsyntax-only -Wredundant-move -std=c++11 -fdiagnostics-parseable-fixits \
%s 2>&1 | FileCheck %s<br> <br>
// definitions for std::move<br>
namespace std {<br>
@@ -102,3 +102,39 @@ D test5(D d) {<br>
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:20}:""<br>
// CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:21-[[@LINE-4]]:22}:""<br>
}<br>
+<br>
+// No more fix-its past here.<br>
+// CHECK-NOT: fix-it<br>
+<br>
+// A deleted copy constructor will prevent moves without std::move<br>
+struct E {<br>
+ E(E &&e);<br>
+ E(const E &e) = delete;<br>
+ // expected-note@-1{{deleted here}}<br>
+};<br>
+<br>
+struct F {<br>
+ F(E);<br>
+ // expected-note@-1{{passing argument to parameter here}}<br>
+};<br>
+<br>
+F test6(E e) {<br>
+ return e;<br>
+ // expected-error@-1{{call to deleted constructor of 'E'}}<br>
+ return std::move(e);<br>
+}<br>
+<br>
+struct G {<br>
+ G(G &&g);<br>
+ // expected-note@-1{{copy constructor is implicitly deleted because 'G' \
has a user-declared move constructor}}<br> +};<br>
+<br>
+struct H {<br>
+ H(G);<br>
+ // expected-note@-1{{passing argument to parameter here}}<br>
+};<br>
+<br>
+H test6(G g) {<br>
+ return g; // expected-error{{call to implicitly-deleted copy constructor of \
'G'}}<br> + return std::move(g);<br>
+}<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
</div></div><span class=""><a href="mailto:cfe-commits@cs.uiuc.edu" \
target="_blank">cfe-commits@cs.uiuc.edu</a><br> <a \
href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" rel="noreferrer" \
target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br> \
</span></blockquote></div><br></div></div> </blockquote></div><br></div></div>
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic