[PATCH] D12839: Extend MoveConstructorInitCheck to also flag constructor arguments passed by value and can be moved assigned to fields.

[Aaron Ballman via cfe-commits]

On Wed, Sep 16, 2015 at 11:41 AM, Alexander Kornienko <alexfh at google.com> wrote:
> On Wed, Sep 16, 2015 at 5:15 PM, Aaron Ballman <aaron.ballman at gmail.com>
> wrote:
>>
>> Okay, I'm sold on this being a clang-tidy check instead, thank you
>> both for weighing in!
>>
>> I would love to see a check that covers more than just constructor
>> initialization contexts, which suggests a separate checker from
>> misc-move-constructor-init so that we don't have the same general idea
>> split over two checkers. For instance, I would love to see this sort
>> of code flagged along with the cases you've identified:
>>
>> struct SomethingBig { /* Stuff */ };
>> struct S {
>>   std::vector<SomethingBig> V;
>> };
>>
>> void f(S &s) {
>>   std::vector<SomethingBig> V;
>>   // Fill out a bunch of elements in V
>>   s.V = V; // suggest std::move()
>> }
>
>
> Detecting that the local variable is not used after something is initialized
> with it will require analysis of the control flow graph in the general case,
> which I'm not sure is currently possible in a clang-tidy check
> (specifically, I'm not sure whether we can make Sema available to checks).

Oye, the number of times I've wished Sema was exposed to clang-tidy is
*huge*. But this is a good point. I'll have to think on it further.

~Aaron

>
>>
>> Btw, I'm not suggesting you have to implement this sort of
>> functionality as part of your patch (but it would be awesome if you
>> were able to!). I'm only thinking about separation of concerns for
>> this.
>>
>> ~Aaron
>>
>> On Wed, Sep 16, 2015 at 3:44 AM, Alexander Kornienko <alexfh at google.com>
>> wrote:
>> > 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" <richard at metafoo.co.uk> wrote:
>> >>
>> >> On Tue, Sep 15, 2015 at 12:50 PM, Aaron Ballman
>> >> <aaron.ballman at 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
>> >>>
>> >>>
>> >>>
>> >>
>> >
>
>