[PATCH] D125402: [clang][diag] warn if function returns class type by-const-value

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D125402#3553855 <https://reviews.llvm.org/D125402#3553855>, @erichkeane wrote:

> In D125402#3553841 <https://reviews.llvm.org/D125402#3553841>, @aaron.ballman wrote:
>
>> In D125402#3553825 <https://reviews.llvm.org/D125402#3553825>, @erichkeane wrote:
>>
>>> In D125402#3553802 <https://reviews.llvm.org/D125402#3553802>, @aaron.ballman wrote:
>>>
>>>> In D125402#3517865 <https://reviews.llvm.org/D125402#3517865>, @nlee wrote:
>>>>
>>>>> How about adding CXXRecordDecl::hasMoveConstructor() to ensure the type is movable? I ran a test with OpenCV(c++11), and it returned some useful warnings:
>>>>
>>>> I don't think that will be quite correct -- IIRC, that would still return true if the move constructor was deleted. `hasSimpleMoveConstructor()` and `hasSimpleMoveAssignment()` might be a better approach.
>>>
>>> The 'Simple' version might not be quite right... That is implemented as:
>>>
>>>   bool hasSimpleMoveConstructor() const {
>>>      return !hasUserDeclaredMoveConstructor() && hasMoveConstructor() &&
>>>             !data().DefaultedMoveConstructorIsDeleted;
>>>    }
>>>
>>>
>>> So this would still warn about user-defined move constructors.
>>
>> Good catch!
>>
>>> HOWEVER, I might suggest `hasMoveConstructor() && !defaultedMoveConstructorIsDeleted()` for the ctor test.  There is similar storage for the 'DefaultedMoveAssignmentIsDeleted`, but it isn't exposed, so you might need to add a function to expose it in DeclCXX.h.
>>
>> I think that might still not be correct because there could be a user-provided move constructor that's explicitly deleted. (So it has a move constructor, but the defaulted one is not deleted because it's user provided.)
>
> I don't think that is possible?  I think 'defaultedMoveConstructor' includes the user typing `StructName(StructName&&) = delete;`.
>
> Note that out of line deleted implementations are prohibited: https://godbolt.org/z/MTK4jGa57 So we don't have to worry about that.

I might be wrong about this above:
https://clang.llvm.org/doxygen/classclang_1_1CXXRecordDecl.html#a8ddbc71ffd71a6f7d580fc7a19452f30
Comment says: " 
/// Set that we attempted to declare an implicit move

/// constructor, but overload resolution failed so we deleted it."

So perhaps more work to figure out non-deleted move ctor, it might require a lookup.  So if we do that, we need a bunch of tests to validate all these conditions I believe (manually deleted, inherited deleted, member-cauesd-deleted, and 'defaulted' versions of the last 2, user provided but works, etc).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125402/new/

https://reviews.llvm.org/D125402