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

[Alexander Kornienko via cfe-commits]

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


> 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
> >>>
> >>>
> >>>
> >>
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150916/b17451dc/attachment-0001.html>