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

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:23
@@ +22,3 @@
+}
+
+void MoveConstantArgumentCheck::check(const MatchFinder::MatchResult &Result) {
----------------
The problem is that each template class or function can have several representations in the AST: one for the template definition and one for each instantiation. Usually, we don't need to even look at the instantiations, when we want to reason about the code in the general case. You can filter out expressions belonging to template instantiations using this narrowing matcher: `unless(isInTemplateInstantiation())`. And for template definitions the type will be marked as dependent.

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:31
@@ +30,3 @@
+
+  bool IsTypeDependOnTemplateParameter = Arg->getType()->isDependentType();
+  if (IsTypeDependOnTemplateParameter)
----------------
The variable here is only used once and its name doesn't make the code much clearer compared to the expression itself. Please remove it.

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:40
@@ +39,3 @@
+    diag(CallMove->getLocStart(), "std::move of the %select{|const "
+                                  "}0%select{expression|variable}1 %select{|of "
+                                  "trivially-copyable type }2has no effect; "
----------------
dvadym wrote:
> Could you please advice how can I correctly make removal? 
> I expected that 
> FixItHint::CreateRemoval(SourceRange(CallMove->getLocStart(), Arg->getLocStart())) removes "std::move(" but it removes "std::move(varname", so from a "move" call only ")" is left
`FixItHint::CreateRemoval` and many other methods accept `CharSourceRange` instead of `SourceRange`. The former is a `SourceRange` + a flag telling whether the range should be treated as a character range or a token range. By default, `SourceRange` is converted to a `CharSourceRange` marked as a token range. So your current code creates a `FixItHint` that removes tokens from `std` to `varname` inclusive. If you want to delete everything from `std` to just before `varname`, you can create a character range from `CallMove->getLocStart()` to `Arg->getLocStart().getLocWithOffset(-1)`.

However, when there's something between `std::move(` and `varname` (whitespace and/or comment(s)), might want to delete just `std::move(`. In this case you can take `CallMove->getCallee()' (which will correspond to `std::move`), and then find the first '(' token after it's end location. It's probably a rare case though, so let's go for the simpler solution for now (with `getLocWithOffset` and character ranges).

================
Comment at: test/clang-tidy/move-const-arg.cpp:1
@@ +1,2 @@
+// RUN: %check_clang_tidy %s misc-move-const-arg %t -- -- -std=c++11
+
----------------
aaron.ballman wrote:
> Please run clang-format over the test files as well.
Did you forget to include the test file to the latest patch? The formatting is still off and the messages don't seem to correspond to the spelling in the code. If you have troubles with clang-format breaking the CHECK lines, be sure to use `clang-format -style=file` so that the test-specific configuration file is used.


http://reviews.llvm.org/D12031