[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 -
----------------
Can `Context` be `const ASTContext &`?


================
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
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp:77
+  // for the function argument being examined.
+  const auto ParamName = Param.getName();
+
----------------
We don't typically use top-level `const` in the project, so you can drop that, and you should only use `auto` when the type is spelled out in the initialization (or is impossible to spell, or strikingly obvious from context like iterators), so you should spell that type out in this case.


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

https://reviews.llvm.org/D87540