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?
~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
