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

[Alexander Kornienko via cfe-commits]

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