alexfh added inline comments.
================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:23
@@ +22,3 @@
+}
+
+void MoveConstantArgumentCheck::check(const MatchFinder::MatchResult &Result) {
----------------
The problem is that each template class or function can have several
representations in the AST: one for the template definition and one for each
instantiation. Usually, we don't need to even look at the instantiations, when
we want to reason about the code in the general case. You can filter out
expressions belonging to template instantiations using this narrowing matcher:
`unless(isInTemplateInstantiation())`. And for template definitions the type
will be marked as dependent.
================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:31
@@ +30,3 @@
+
+ bool IsTypeDependOnTemplateParameter = Arg->getType()->isDependentType();
+ if (IsTypeDependOnTemplateParameter)
----------------
The variable here is only used once and its name doesn't make the code much
clearer compared to the expression itself. Please remove it.
================
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; "
----------------
dvadym wrote:
> 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
`FixItHint::CreateRemoval` and many other methods accept `CharSourceRange`
instead of `SourceRange`. The former is a `SourceRange` + a flag telling
whether the range should be treated as a character range or a token range. By
default, `SourceRange` is converted to a `CharSourceRange` marked as a token
range. So your current code creates a `FixItHint` that removes tokens from
`std` to `varname` inclusive. If you want to delete everything from `std` to
just before `varname`, you can create a character range from
`CallMove->getLocStart()` to `Arg->getLocStart().getLocWithOffset(-1)`.
However, when there's something between `std::move(` and `varname` (whitespace
and/or comment(s)), might want to delete just `std::move(`. In this case you
can take `CallMove->getCallee()' (which will correspond to `std::move`), and
then find the first '(' token after it's end location. It's probably a rare
case though, so let's go for the simpler solution for now (with
`getLocWithOffset` and character ranges).
================
Comment at: test/clang-tidy/move-const-arg.cpp:1
@@ +1,2 @@
+// RUN: %check_clang_tidy %s misc-move-const-arg %t -- -- -std=c++11
+
----------------
aaron.ballman wrote:
> Please run clang-format over the test files as well.
Did you forget to include the test file to the latest patch? The formatting is
still off and the messages don't seem to correspond to the spelling in the
code. If you have troubles with clang-format breaking the CHECK lines, be sure
to use `clang-format -style=file` so that the test-specific configuration file
is used.
http://reviews.llvm.org/D12031
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
