[PATCH] D50102: [clang-tidy] Use ExprMutationAnalyzer in performance-unnecessary-value-param

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang added inline comments.


================
Comment at: clang-tidy/performance/UnnecessaryValueParamCheck.cpp:108
     return;
+  if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(Function)) {
+    for (const auto *Init : Ctor->inits()) {
----------------
hokein wrote:
> Is this a new fix or  a special case not being caught by `ExprMutationAnalyzer`?  Do we have a test case covered it?
It's a special case.
ExprMutationAnalyzer is designed to analyze within a "scope" indicated by a Stmt. So when trying to analyze a "function", merely looking at function body is not sufficient because CXXCtorInitializer is not part of the function body. So far we only care about this special case when trying to figure out whether a function param is mutated or not, which is exactly what this check is doing. We might consider making this part of ExprMutationAnalyzer but IMO that depends on whether there are more use cases of this special case in the future because we'll need some tricks there to match different types of top level nodes.

This is already captured in existing unit test case, which basically looks like this:
```
class Foo {
public:
  Foo(ExpensiveObj x) : x(std::move(x)) {}
private:
  ExpensiveObj x;
};
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50102