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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
