alexfh added a comment.
Thank you for the update! Looks almost right. A few minor comments.
================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:34
@@ +33,3 @@
+ const Expr *Arg = CallMove->getArg(0);
+ auto *Context = Result.Context;
+
----------------
Looks like you're using `Context` once and all the other times you need
`Context->getSourceManager()`. I suggest pulling it to a local variable
(`SourceManager &SM` or `SourceManager &Sources` - whatever you like more) and
replace the usage of `Context` with `Result.Context`.
================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:40
@@ +39,3 @@
+ if (IsConstArg || IsTriviallyCopyable) {
+ auto MoveRange = CharSourceRange::getCharRange(CallMove->getLocStart(),
+ CallMove->getLocEnd());
----------------
You should be able to use
`CharSourceRange::getCharRange(CallMove->getSourceRange())` instead. It's
slightly more compact.
================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:45
@@ +44,3 @@
+ if (!FileMoveRange.isValid()) {
+ // Do nothing if this move call is in a macro expansion.
+ return;
----------------
Remove the comment, it's now wrong.
================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:51
@@ +50,3 @@
+ diag(FileMoveRange.getBegin(),
+ "std::move of the %select{|const "
+ "}0%select{expression|variable}1 %select{|of "
----------------
nit: clang-format has done a poor job here: spaces are not the best places to
break this string literal. Please manually reformat this literal so that the
`%select{...}N` constructs are not broken. Thanks!
================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:54
@@ +53,3 @@
+ "trivially-copyable type }2has no effect; "
+ "remove std::move().")
+ << IsConstArg << IsVariable << IsTriviallyCopyable
----------------
Diagnostic messages are not full sentences and thus require neither
capitalization (already fine here) nor trailing period.
================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:56
@@ +55,3 @@
+ << IsConstArg << IsVariable << IsTriviallyCopyable
+ << FixItHint::CreateRemoval(Lexer::makeFileCharRange(
+ CharSourceRange::getCharRange(CallMove->getLocStart(),
----------------
After some thinking, there may be cases where the range of the whole expression
can be translated to a file char range, but a sub-range of it can't. So you
need to check the validity of the results of both `makeFileCharRange` calls in
this expression before creating a fixit hint with the resulting ranges.
Also, it seems reasonable to issue a warning in any case, and fix-it hints only
when we are rather certain that we can apply them safely (which the validity of
all `makeFileCharRange` results should tell us about).
================
Comment at: test/clang-tidy/move-const-arg.cpp:26
@@ +25,3 @@
+ return std::move(42);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the expression of
+ // trivially-copyable type has no effect; remove std::move().
----------------
I see that clang-format did a wrong thing here. It should have used the
`tools/clang/tools/extra/test/.clang-format` configuration file containing the
`ColumnLimit: 0` setting, which disables column limit enforcement. Please merge
the `CHECK-MESSAGES` lines (otherwise, the second line is just ignored) and in
the future use `clang-format -style=file` when reformatting test files.
Please also specify the full diagnostic message once including the
[check-name]. The rest messages can be truncated to remove duplicating text
(e.g. remove the `has no effect; remove std::move().` substring).
================
Comment at: test/clang-tidy/move-const-arg.cpp:54
@@ +53,3 @@
+
+template <typename T> T f6(const T x6) { return std::move(x6); }
+
----------------
Please add a test that insures that correct fixes are applied to template
functions and only once, e.g. like this:
```
template <typename T> T f(const int x) {
return std::move(x);
// CHECK-FIXES: return x;
}
void g() {
f<int>(1);
f<double>(1);
}
```
http://reviews.llvm.org/D12031
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
