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

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:11
@@ -10,2 +10,3 @@
 #include "MoveConstructorInitCheck.h"
+#include "../utils/Matchers.h"
 #include "clang/AST/ASTContext.h"
----------------
Errr, I'm not certain that we typically use relative paths for this sort of thing. I see we do use relative paths for other things in clang-tidy, but this is definitely an uncommon thing in the rest of the code base (outside of test or example code).

Not saying you should change anything here, but we may want to consider changing the paths at some point so we are more in line with the rest of the LLVM projects in not using relative include paths.

================
Comment at: clang-tidy/utils/Matchers.h:1
@@ +1,2 @@
+//===--- Matchers.h - clang-tidy-------------------------------------------===//
+//
----------------
You are missing header include guards.

================
Comment at: clang-tidy/utils/TypeTraits.cpp:29
@@ +28,3 @@
+  // excessive copying, we'll warn on them.
+  if (Type->isDependentType())
+    return false;
----------------
This isn't needed because Type.isTriviallyCopyableType() already returns false for this case.

================
Comment at: clang-tidy/utils/TypeTraits.cpp:32
@@ +31,3 @@
+  // Ignore trivially copyable types.
+  return !(Type->isScalarType() || Type.isTriviallyCopyableType(Context) ||
+           classHasTrivialCopyAndDestroy(Type));
----------------
No need to check for isScalarType() because isTriviallyCopyableType() already does that correctly.

================
Comment at: clang-tidy/utils/TypeTraits.h:22
@@ +21,3 @@
+/// destructor.
+bool classHasTrivialCopyAndDestroy(QualType Type);
+
----------------
I think this is an implementation detail more than a trait we want to expose.

================
Comment at: test/clang-tidy/misc-move-constructor-init.cpp:114
@@ +113,3 @@
+  TriviallyCopyable T_;
+  int I_;
+};
----------------
Sorry to have not caught this earlier, but can you also add a test case for dependent types to make sure they remain negative?


http://reviews.llvm.org/D12839