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

[Vadym Doroshenko via cfe-commits]

dvadym added a comment.

Thanks alexfh and sbenza for comments!
I've addressed most of them. Could you please advice how to find that some expression has type which is template dependent?

Regards,
Vadym


================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:20
@@ +19,3 @@
+  const auto* CallMove = result.Nodes.getNodeAs<CallExpr>("call-move");
+  if (CallMove->getNumArgs() != 1) return;
+  const Expr* Arg = CallMove->getArg(0);
----------------
sbenza wrote:
> You can move both checks to the matcher.
> Something like:
> 
>     callExpr(callee(functionDecl(hasName("::std::move"))),
>              argumentCountIs(1),
>              hasArgument(0, expr(hasType(isConstQualified()))))
Thanks, according to alexfh comments I've decided to apply this check not only for const but also for trivially copyable types.

================
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);
----------------
alexfh wrote:
> 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".
Thanks, I've addressed your comments.

================
Comment at: test/clang-tidy/move-const-arg.cpp:29
@@ +28,3 @@
+{
+  return std::move(42);
+}
----------------
alexfh wrote:
> 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.
Thanks, I've implemented for trivially copyable types. But I didn't find how to find dependent types: Arg->isTypeDependent(), Arg->isInstantiationDependent(), Arg->getType()->isDependentType() doesn't look like solving this problem. Could you please advice?

================
Comment at: test/clang-tidy/move-const-arg.cpp:41
@@ +40,3 @@
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: move of const variable [move-const-arg]
+  // CHECK-FIXES: return x;
+}
----------------
alexfh wrote:
> Please make the checked code lines unique to avoid matching in a wrong place. FileCheck (the utility that is called by the check_clang_tidy.py script to verify the `// CHECK-...` lines, http://llvm.org/docs/CommandGuide/FileCheck.html) doesn't take the location of the `// CHECK-FIXES:` line in the test file into consideration, it just verifies that the fixed file has a subsequence of lines that matches all `// CHECK-FIXES` lines in that order. Here, for example, `return x;` would equally match, if the check would fix line 34 instead of line 39. We could solve this by numbering lines so that CHECK-FIXES patterns could refer to the line numbers, but until we do that (if we decide so), making the checked lines unique is the way to go (e.g. by using different variable names in each test case instead of just `x`).
Thanks, I've set different names to variables


http://reviews.llvm.org/D12031