I uploaded a patch at http://reviews.llvm.org/D11615
On Wed, Jul 29, 2015 at 4:30 PM, Richard Smith <rich...@metafoo.co.uk> wrote: > On Wed, Jul 29, 2015 at 2:55 PM, Richard Trieu <rtr...@google.com> wrote: > >> On Wed, Jul 29, 2015 at 1:52 PM, Richard Smith <rich...@metafoo.co.uk> >> wrote: >> >>> On Tue, Jul 28, 2015 at 12:06 PM, Richard Trieu <rtr...@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 >>>> >>> >>> >> >
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits