[PATCH] D12031: Const std::move() argument ClangTidy check

[Aaron Ballman via cfe-commits]

aaron.ballman added a subscriber: aaron.ballman.

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:1
@@ +1,2 @@
+#include "MoveConstantArgumentCheck.h"
+
----------------
Missing new file legal text.

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:9
@@ +8,3 @@
+
+void MoveConstantArgumentCheck::registerMatchers(MatchFinder* Finder) {
+  Finder->addMatcher(
----------------
Formatting -- the * should go with Finder. May want to run clang-format over the entire patch; there's a lot of other formatting issues I will refrain from commenting on.

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:10
@@ +9,3 @@
+void MoveConstantArgumentCheck::registerMatchers(MatchFinder* Finder) {
+  Finder->addMatcher(
+      callExpr(callee(functionDecl(hasName("::std::move")))).bind("call-move"),
----------------
Please only register this matcher for C++ code.

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:22
@@ +21,3 @@
+  bool IsTypeDependOnTemplateParameter =
+      false;  // my first guess was type->getTypeClass () == 30 but it doesn't
+              // work in some cases. Could you please advice better solution.
----------------
Arg->getType()->isDependentType() should do what you want, if I understand you properly.

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:29
@@ +28,3 @@
+  if (IsConstArg || IsTriviallyCopyable) {
+    bool IsVariable = dyn_cast<DeclRefExpr>(Arg) != nullptr;
+    std::string message = "std::move of the ";
----------------
isa<DeclRefExpr> instead of dyn_cast and check against nullptr.

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:34
@@ +33,3 @@
+    message += IsTriviallyCopyable ? "of trivially-copyable type " : "";
+    message += "doesn't have effect. ";
+    message += "Remove std::move()";
----------------
This line reads a bit strangely to me. Perhaps "has no effect" instead? Also, our diagnostics are not full sentences, so you should remove the period, and instead do: "; remove std::move()"

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.h:1
@@ +1,2 @@
+#include "../ClangTidy.h"
+
----------------
Missing header include guards and header legal text.


http://reviews.llvm.org/D12031