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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
