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

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Thank you for tackling this!

A high-level comment: the check needs to be somewhat more general. Const-qualified variables are just a specific case of an rvalue. The check should warn on all usages of std::move with an rvalue argument (except in templates with arguments of dependent types).

Additionally, I would extend the check (arguably, this should be a separate check) to complain about use of std::move with trivially-copyable types, as it seems that there's no reason to prefer moving for these types anyway (again, with the exception of dependent types in templates).


================
Comment at: clang-tidy/misc/MiscTidyModule.cpp:70
@@ -68,2 +69,3 @@
     CheckFactories.registerCheck<UseOverrideCheck>("misc-use-override");
+    CheckFactories.registerCheck<MoveConstantArgumentCheck>("move-const-arg");
   }
----------------
The name should start with "misc-". Please also update the position of the statement to maintain sorting.

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:3
@@ +2,3 @@
+
+#include<iostream>
+using namespace std;
----------------
I suppose this is not needed. The line below as well.

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:18
@@ +17,3 @@
+
+void MoveConstantArgumentCheck::check(const MatchFinder::MatchResult& result) {
+  const auto* CallMove = result.Nodes.getNodeAs<CallExpr>("call-move");
----------------
Please follow LLVM naming convention (http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly). s/result/Result/

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:24
@@ +23,3 @@
+  if (Arg->getType().isConstQualified()) {
+    SourceManager* sm = result.SourceManager;
+    SourceRange MoveRange(CallMove->getLocStart(), CallMove->getRParenLoc());
----------------
s/SourceManager* sm/SourceManager &SM/

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:26
@@ +25,3 @@
+    SourceRange MoveRange(CallMove->getLocStart(), CallMove->getRParenLoc());
+    clang::SourceLocation ArgBegin(Arg->getLocStart()),
+        ArgEnd(Arg->getLocEnd());
----------------
No need for "clang::" as the code is inside the clang namespace.

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:31
@@ +30,3 @@
+    std::string ArgString(sm->getCharacterData(ArgBegin), length);
+    diag(CallMove->getLocStart(), "move of const variable")
+        << FixItHint::CreateReplacement(MoveRange, ArgString);
----------------
The message could be more helpful. For example, "std::move of the const variable '%0' doesn't have effect". We could also add a recommendation on how to fix that (e.g. "remove std::move() or make the variable non-const").

Also, in case it's not a variable (DeclRefExpr), the warning shouldn't say "variable".

================
Comment at: test/clang-tidy/move-const-arg.cpp:29
@@ +28,3 @@
+{
+  return std::move(42);
+}
----------------
That also doesn't look like a reasonable use of std::move. We should probably extend this check to flag std::move applied to rvalues (in general, not only usages of const variables), scalar types, pointer types and probably also all other trivially-copyable types (I don't immediately see why moving those could ever be better than copying). These warnings shouldn't trigger for dependent types and in template instantiations.


http://reviews.llvm.org/D12031