riccibruno added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp:74
+ // parmVarDecl picked up by this checker. It will be an empty string and will
+ // lead to an assertion failure when using hasName(std::string) being used
+ // in the matcher below. If empty then exit indicating no move calls present
----------------
aaron.ballman wrote:
> This may be a bigger issue with `hasName()` as this strikes me as possibly
> being a bug. I would expect `hasName("")` to return `false` for any AST node
> which has a nonempty name, and `true` for any AST node without a name.
When I was looking at the `hasName` matcher I was surprised that this is not
the case (see `getNodeName` in `ASTMatchers/ASTMatchersInternal.cpp`). For
example an unnamed enumeration can be matched with `(anonymous enum)` (which
should be `unnamed` instead). For an other example a constructor can be matched
with the name of the class despite the fact that the constructor is formally
unnamed (because `DeclarationName::getDeclName` and `NamedDecl::printName` are
used).
I think that the `hasName` matcher is mixing two different concepts: the formal
name of the AST node and the name for diagnostic purposes. One possible fix
would be to add a matcher `hasFormalName` which would match the name as per the
specification, and then modify the `hasName` matcher to use the name for
diagnostic purposes (without the extra location information).
Not hard-coding the logic in `getNodeName` would have the additional benefit of
being more consistent with the use of `anonymous`/`unnamed` terminology.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87540/new/
https://reviews.llvm.org/D87540
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
