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