<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_ASROnREeq0cCxk&m=_zqU25z8o2K8Mmr_S9WUCUnJKxAgFGbkHNxNEiYNMK0&s=jmB8EV-XHCHBmAI_rPlijldEaWAwQcmkXORyVeKIhLc&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><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-3D243462-26r2-3D243463-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=_zqU25z8o2K8Mmr_S9WUCUnJKxAgFGbkHNxNEiYNMK0&s=ebHNf-UhXYtrumMNNUSbS5LXTi7fj-c9E74Rl0l0Zfs&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/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>