[PATCH] D87540: [clang-tidy] Fix false positive issue in performance-unnecessary-value-param for arguments being moved in the function body.

[Bruno Ricci via Phabricator via cfe-commits]

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