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