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

[Richard Smith]

On Wed, Jul 29, 2015 at 2:55 PM, Richard Trieu <rtrieu at google.com> wrote:

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

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