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