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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp:66
 
+bool isPassedToStdMove(const ParmVarDecl &Param, ASTContext &Context) {
+  // Check if the parameter has a name, in case of functions like -
----------------
sukraat91 wrote:
> aaron.ballman wrote:
> > Can `Context` be `const ASTContext &`?
> Unfortunately, no. This is because match() does not have an overload for `const ASTContext&`.
I kind of wondered if that was the case, oh well. Thank you for checking!


================
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
----------------
riccibruno wrote:
> 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.
+1, I think that's the better fix to make here. I don't have the impression that matching names for diagnostic purposes was really intended for this API (I don't recall discussions about it). Based on that, I think `hasName()` should probably be the one that handles the identifier used by the AST node and we can add `hasDiagnosticName()` to handle matching things like anonymous enums by name when we find there's a need for it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87540/new/

https://reviews.llvm.org/D87540