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

Haojian Wu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Aug 3 01:12:51 PDT 2018

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


Comment at: clang-tidy/performance/UnnecessaryValueParamCheck.cpp:108
+  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?

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list