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

[Richard Trieu]

I uploaded a patch at http://reviews.llvm.org/D11615

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

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