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

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.

================
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.
----------------
aaron.ballman wrote:
> Arg->getType()->isDependentType() should do what you want, if I understand you properly.
Yep, should be what you need.

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

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:39
@@ +38,3 @@
+
+    SourceRange RemoveRange1(CallMove->getLocStart(), Arg->getLocStart());
+    SourceRange RemoveRange2(CallMove->getLocEnd(), CallMove->getLocEnd());
----------------
The variable names don't add much value here. Either remove the variables and use the expressions below or give the variables more useful names. Also note that `SourceRange` can be constructed from a single token (`SourceRange(CallMove->getLocEnd())`).

================
Comment at: test/clang-tidy/move-const-arg.cpp:1
@@ +1,2 @@
+// RUN: $(dirname %s)/check_clang_tidy.sh %s misc-move-const-arg %t
+
----------------
This has changed once more, now `%check_clang_tidy` should be used instead of the script itself. Please also rebase the patch on top of current HEAD.

================
Comment at: test/clang-tidy/move-const-arg.cpp:4
@@ +3,3 @@
+namespace std {
+// Directly copied from the stl.
+template<typename>
----------------
The comment is not useful here. This is a mock implementation, and it's not important where does this come from. Please also clang-format the test with `-style=file` (to pick up formatting options specific to tests, namely unlimited columns limit).

================
Comment at: test/clang-tidy/move-const-arg.cpp:30
@@ +29,3 @@
+  A() {}
+  A(const A& rhs) {}
+  A(A&& rhs) {}
----------------
How did you find that `Arg->getType()->isDependentType()` doesn't work?


http://reviews.llvm.org/D12031