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

[Richard Trieu]

On Wed, Jul 29, 2015 at 1:52 PM, Richard Smith <richard at metafoo.co.uk>
wrote:

> 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.
>

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 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/4a590f8c/attachment.html>