This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Use ExprMutationAnalyzer in performance-unnecessary-value-param
ClosedPublic

Authored by shuaiwang on Jul 31 2018, 2:07 PM.

Details

Summary

This yields better recall as ExprMutationAnalyzer is more accurate.

One common pattern this check is now able to catch is:

void foo(std::vector<X> v) {
  for (const auto& elm : v) {
    // ...
  }
}

Event Timeline

shuaiwang created this revision.Jul 31 2018, 2:07 PM

It is great when the test cases are included :)

JonasToth retitled this revision from Use ExprMutationAnalyzer in performance-unnecessary-value-param to [clang-tidy] Use ExprMutationAnalyzer in performance-unnecessary-value-param.Jul 31 2018, 3:20 PM
shuaiwang updated this revision to Diff 158408.Jul 31 2018, 3:45 PM

Add test case

hokein added a comment.Aug 1 2018, 4:11 AM

Mostly good. A few nits.

clang-tidy/performance/UnnecessaryValueParamCheck.cpp
103

This comment needs to be updated.

108

Is this a new fix or a special case not being caught by ExprMutationAnalyzer? Do we have a test case covered it?

shuaiwang updated this revision to Diff 158856.Aug 2 2018, 3:57 PM
shuaiwang marked 2 inline comments as done.

Update comments.

shuaiwang added inline comments.Aug 2 2018, 3:58 PM
clang-tidy/performance/UnnecessaryValueParamCheck.cpp
108

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;
};
hokein accepted this revision.Aug 3 2018, 1:12 AM

LGTM.

clang-tidy/performance/UnnecessaryValueParamCheck.cpp
108

Thanks for the detailed clarification! That makes sense. Could you also add a proper comment in the code?

This revision is now accepted and ready to land.Aug 3 2018, 1:12 AM
shuaiwang updated this revision to Diff 159037.Aug 3 2018, 10:18 AM
shuaiwang marked 2 inline comments as done.

Add comments explaining CXXCtorInitializer check

This revision was automatically updated to reflect the committed changes.