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

[Vadym Doroshenko via cfe-commits]

dvadym added a comment.

Thanks for review comments! Could you please have an another look and help me with my questions?


================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:10
@@ +9,3 @@
+void MoveConstantArgumentCheck::registerMatchers(MatchFinder* Finder) {
+  Finder->addMatcher(
+      callExpr(callee(functionDecl(hasName("::std::move")))).bind("call-move"),
----------------
aaron.ballman wrote:
> Please only register this matcher for C++ code.
I didn't find how it can be done, could you please advice?

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:22
@@ +21,3 @@
+  bool IsTypeDependOnTemplateParameter =
+      false;  // my first guess was type->getTypeClass () == 30 but it doesn't
+              // work in some cases. Could you please advice better solution.
----------------
alexfh wrote:
> aaron.ballman wrote:
> > Arg->getType()->isDependentType() should do what you want, if I understand you properly.
> Yep, should be what you need.
It doesn't do what it's needed. See for example function f6, f7, f8 in tests. ::check method is called once on any instantion of f6, Arg->getType()->isDependentType() returns false, so  the check returns 2 warning.

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:29
@@ +28,3 @@
+  if (IsConstArg || IsTriviallyCopyable) {
+    bool IsVariable = dyn_cast<DeclRefExpr>(Arg) != nullptr;
+    std::string message = "std::move of the ";
----------------
aaron.ballman wrote:
> isa<DeclRefExpr> instead of dyn_cast and check against nullptr.
Thanks

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:30
@@ +29,3 @@
+    bool IsVariable = dyn_cast<DeclRefExpr>(Arg) != nullptr;
+    std::string message = "std::move of the ";
+    message += IsConstArg ? "const " : "";
----------------
alexfh wrote:
> alexfh wrote:
> > Please don't string += as a way to build messages. This creates a temporary each time and reallocates the string buffer. Use one of the these:
> >   
> >   std::string message = (llvm::Twine("...") + "..." + "...").str()
> > 
> > (only in a single expression, i.e. don't create variables of the llvm::Twine type, as this can lead to dangling string references), or:
> > 
> >   std::string buffer;
> >   llvm::raw_string_ostream message(buffer);
> >   message << "...";
> >   message << "...";
> >   // then use message.str() where you would use an std::string.
> > 
> > The second alternative would be more suitable for this kind of code.
> > 
> > BUT, even this is not needed in the specific case of producing diagnostics, as clang provides a powerful template substitution mechanism, and your code could be written more effectively:
> > 
> >   diag("std::move of the %select{const |}0 %select{variable|expression}1 ...") << IsConstArg << IsVariable << ...;
> This should read:
> "Please don't use string += as a way to build messages. This creates a temporary each time and reallocates the string buffer. Instead, use one of these patterns:"
Thanks, it made code much clearer

================
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; "
----------------
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


http://reviews.llvm.org/D12031