I also think this suits better for a clang-tidy check, but for a different
reason: adding `std::move` calls can require adding `#include <utility>`,
which is fine for a clang-tidy check (and we have facilities for that), but
it's a questionable functionality for a compiler diagnostic.
On 16 Sep 2015 09:20, "Richard Smith" <rich...@metafoo.co.uk> wrote:
> On Tue, Sep 15, 2015 at 12:50 PM, Aaron Ballman <aaron.ball...@gmail.com>
> wrote:
>
>> aaron.ballman added reviewers: rtrieu, rsmith.
>> aaron.ballman added a comment.
>>
>> There is a -Wpessimizing-move frontend warning that Richard added not too
>> long ago, which tells the user to remove calls to std::move() because they
>> pessimize the code. The new functionality you are looking to add is
>> basically the opposite: it tells the user to add std::move() because the
>> code is currently pessimized due to copies. Given how general that concept
>> is (it doesn't just appertain to constructor initializations), I wonder if
>> this makes more sense as a frontend warning like -Wpessimizing-copy.
>>
>> Richard (both of you), what do you think?
>>
>
> I think this is in the grey area between what's appropriate for clang-tidy
> and what's appropriate as a warning, where both options have merit; the
> same is true with -Wpessimizing-move. I think the difference between the
> two cases is that -Wpessimizing-move detects a case where you wrote
> something that doesn't do what (we think) you meant, whereas this check
> detects a case where you /didn't/ write something that (we think) would
> make your code better (even though both changes have similar effects, of
> safely turning a copy into a move or a move into an elided move). It's a
> fine line, but for me that nudges -Wpessimizing-move into a warning, and
> this check into clang-tidy territory.
>
> ~Aaron
>>
>>
>> ================
>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:32
>> @@ +31,3 @@
>> + // excessive copying, we'll warn on them.
>> + if (Node->isDependentType()) {
>> + return false;
>> ----------------
>> Elide braces, here and elsewhere.
>>
>> ================
>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:36
>> @@ +35,3 @@
>> + // Ignore trivially copyable types.
>> + if (Node->isScalarType() ||
>> + Node.isTriviallyCopyableType(Finder->getASTContext()) ||
>> ----------------
>> Can turn this into a return statement directly instead of an if.
>>
>> ================
>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:38
>> @@ +37,3 @@
>> + Node.isTriviallyCopyableType(Finder->getASTContext()) ||
>> + classHasTrivialCopyAndDestroy(Node)) {
>> + return false;
>> ----------------
>> Why do you need classHasTrivialCopyAndDestroy() when you're already
>> checking if it's a trivially copyable type?
>>
>> ================
>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:44
>> @@ +43,3 @@
>> +
>> +int parmVarDeclRefExprOccurences(const ParmVarDecl &MovableParam,
>> + const CXXConstructorDecl
>> &ConstructorDecl,
>> ----------------
>> Should return unsigned, please.
>>
>> ================
>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:50
>> @@ +49,3 @@
>> + findAll(declRefExpr(to(parmVarDecl(equalsNode(&MovableParam)))));
>> + auto Matches = match(AllDeclRefs, *ConstructorDecl.getBody(), Context);
>> + Occurrences += Matches.size();
>> ----------------
>> You don't actually need Matches, you can call match().size() instead.
>>
>> ================
>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:52
>> @@ +51,3 @@
>> + Occurrences += Matches.size();
>> + for (const auto* Initializer : ConstructorDecl.inits()) {
>> + Matches = match(AllDeclRefs, *Initializer->getInit(), Context);
>> ----------------
>> Should be *Initializer instead of * Initializer.
>>
>> ================
>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:120
>> @@ +119,3 @@
>> + }
>> + diag(InitArg->getLocStart(), "value parameter can be moved to avoid
>> copy.");
>> +}
>> ----------------
>> Perhaps: "argument can be moved to avoid a copy" instead?
>>
>> ================
>> Comment at: test/clang-tidy/misc-move-constructor-init.cpp:84
>> @@ +83,3 @@
>> + Movable(const Movable&) = default;
>> + Movable& operator =(const Movable&) = default;
>> + ~Movable() {}
>> ----------------
>> Formatting
>>
>> ================
>> Comment at: test/clang-tidy/misc-move-constructor-init.cpp:85
>> @@ +84,3 @@
>> + Movable& operator =(const Movable&) = default;
>> + ~Movable() {}
>> +};
>> ----------------
>> Why not = default?
>>
>> ================
>> Comment at: test/clang-tidy/misc-move-constructor-init.cpp:113
>> @@ +112,3 @@
>> +
>> +struct NegativeParamTriviallyCopyable {
>> + NegativeParamTriviallyCopyable(TriviallyCopyable T) : T_(T) {}
>> ----------------
>> Should also have a test for scalars
>>
>>
>> http://reviews.llvm.org/D12839
>>
>>
>>
>>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
