[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 Smith <richard () metafoo ! co ! uk>
Date: 2015-07-29 23:30:50
Message-ID: CAOfiQqkbGws2bAjNGNdfbyp1GLVokOzAm63tSCpGuNrGeUFnPg () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On Wed, 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/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<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"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jul 29, 2015 \
at 2:55 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com" \
target="_blank">rtrieu@google.com</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"><div><div class="h5">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>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>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=_zqU25z8o2K8Mmr_S9WUCUnJKxAgFGbkHNxNEiYNMK0&s=a5MDdWf-V_wo6hRORSrJV79WTpnRkOOZq4snssdyzrE&e=" \
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=243463&view=rev</a><span><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=_zqU25z8o2K8Mmr_S9WUCUnJKxAgFGbkHNxNEiYNMK0&s=jmB8EV-XHCHBmAI_rPlijldEaWAwQcmkXORyVeKIhLc&e=" \
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sem \
a/SemaInit.cpp?rev=243463&r1=243462&r2=243463&view=diff</a><span><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></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></div></blockquote><div><br></div><div>Ah yes, you're \
right, we should keep the -Wredundant-move warning for that case.</div><div><span \
style="color:rgb(80,0,80)"> </span></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 \
class="h5"><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"><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex"><span> + }<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=_zqU25z8o2K8Mmr_S9WUCUnJKxAgFGbkHNxNEiYNMK0&s=ebHNf-UhXYtrumMNNUSbS5LXTi7fj-c9E74Rl0l0Zfs&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><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><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></div></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