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