r243463 - Do not give a -Wredundant-move warning when removing the move will result in an

[Richard Smith]

On Tue, Jul 28, 2015 at 12:06 PM, Richard Trieu <rtrieu at 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.


> +        }
> +      }
> +    }
> +
>      // 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 at -1{{deleted here}}
> +};
> +
> +struct F {
> +  F(E);
> +  // expected-note at -1{{passing argument to parameter here}}
> +};
> +
> +F test6(E e) {
> +  return e;
> +  // expected-error at -1{{call to deleted constructor of 'E'}}
> +  return std::move(e);
> +}
> +
> +struct G {
> +  G(G &&g);
> +  // expected-note at -1{{copy constructor is implicitly deleted because
> 'G' has a user-declared move constructor}}
> +};
> +
> +struct H {
> +  H(G);
> +  // expected-note at -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 at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150729/344db822/attachment.html>