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

[Vadym Doroshenko via cfe-commits]

dvadym added a comment.

Thanks for comments! PTAL
Since it's added checking of trivially copyable arguments of move, it's needed to rename this check, what do you think about MoveNoEffectCheck?


================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:11
@@ +10,3 @@
+#include "MoveConstantArgumentCheck.h"
+
+namespace clang {
----------------
aaron.ballman wrote:
> > I didn't find how it can be done, could you please advice?
> 
> This is the usual way we do it (in the registerMatchers() function):
> ```
>   if (!getLangOpts().CPlusPlus)
>     return;
> ```
> 
Thanks

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:23
@@ +22,3 @@
+}
+
+void MoveConstantArgumentCheck::check(const MatchFinder::MatchResult &Result) {
----------------
alexfh wrote:
> 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.
Great, thank you. It works

================
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; "
----------------
alexfh wrote:
> 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).
Thanks, creating CharSourceRange makes the trick. Also as we talked offline I've added a call of Lexer::makeFileCharRange( for processing move call in macros. Please have a look.


http://reviews.llvm.org/D12031