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

[Felix Berger via cfe-commits]

flx added inline comments.

================
Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:38
@@ +37,3 @@
+      Node.isTriviallyCopyableType(Finder->getASTContext()) ||
+      classHasTrivialCopyAndDestroy(Node)) {
+    return false;
----------------
aaron.ballman wrote:
> Why do you need classHasTrivialCopyAndDestroy() when you're already checking if it's a trivially copyable type?
We also want to catch types that have non-trivial destructors which would be executed when the temporary copy goes out of scope.

================
Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:44
@@ +43,3 @@
+
+int parmVarDeclRefExprOccurences(const ParmVarDecl &MovableParam,
+                                 const CXXConstructorDecl &ConstructorDecl,
----------------
aaron.ballman wrote:
> Should return unsigned, please.
Done. Is this an llvm convention?

================
Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:120
@@ +119,3 @@
+  }
+  diag(InitArg->getLocStart(), "value parameter can be moved to avoid copy.");
+}
----------------
alexfh wrote:
> alexfh wrote:
> > aaron.ballman wrote:
> > > Perhaps: "argument can be moved to avoid a copy" instead?
> > nit: Please remove the trailing period.
> Does anything stop us from suggesting fixes here (inserting "std::move()" around the argument and #include <utility>, if it's no there yet)?
How would I tread in the IncludeOrder style (i.e. Google vs LLVM)? Should this be a flag shared by all of ClangTidy or specific to this check?

================
Comment at: test/clang-tidy/misc-move-constructor-init.cpp:85
@@ +84,3 @@
+  Movable& operator =(const Movable&) = default;
+  ~Movable() {}
+};
----------------
aaron.ballman wrote:
> Why not = default?
We need to make the type non-trivially copyable by our definition above.

================
Comment at: test/clang-tidy/misc-move-constructor-init.cpp:113
@@ +112,3 @@
+
+struct NegativeParamTriviallyCopyable {
+  NegativeParamTriviallyCopyable(TriviallyCopyable T) : T_(T) {}
----------------
aaron.ballman wrote:
> Should also have a test for scalars
Added.


http://reviews.llvm.org/D12839