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. > > >> + } >> + } >> + } >> + >> // 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
