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