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