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

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: clang-tidy/performance/UnnecessaryValueParamCheck.cpp:108
     return;
+  if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(Function)) {
+    for (const auto *Init : Ctor->inits()) {
----------------
shuaiwang wrote:
> 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;
> };
> ```
Thanks for the detailed clarification! That makes sense. Could you also add a proper comment in the code?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50102