Page MenuHomePhabricator

ForRangeCopyCheck that warns on and fixes unnecessary copies of loop variables.
ClosedPublic

Authored by flx on Oct 18 2015, 4:28 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

flx updated this revision to Diff 37702.Oct 18 2015, 4:28 AM
flx retitled this revision from to ForRangeCopyCheck that warns on and fixes unnecessary copies of loop variables..
flx updated this object.
flx added reviewers: alexfh, klimek.
alexfh updated this object.Nov 5 2015, 1:40 PM
alexfh edited edge metadata.Nov 5 2015, 2:00 PM

Thank you for the patch! One of its dependencies (D13845) is not yet in, but you can in the meantime polish this patch.

clang-tidy/misc/ForRangeCopyCheck.cpp
97 ↗(On Diff #37702)

How about a solution that doesn't create unnecessary temporary sets? Something similar to:

std::set<const DeclRefExpr *> Nodes = extractNodesById<...>(...);
DeclRefs.insert(Nodes.begin(), Nodes.end());
Matches = ...;
Nodes = extractNodesById...;
DeclRefs.insert(Nodes.begin(), Nodes.end());
return DeclRefs;

?

116 ↗(On Diff #37702)

I'd remove the variable and use the expression itself below.

141 ↗(On Diff #37702)

nit: It's not common to use braces around single-line for/if bodies in LLVM.

151 ↗(On Diff #37702)

Use isa<AutoType>(...) instead of dyn_cast<...>(...) == nullptr.

154 ↗(On Diff #37702)

Remove the braces around if, so it becomes else if.

189 ↗(On Diff #37702)

setDifference is only used once, and only to check whether it's empty. I'd prefer to replace it with setDifferenceEmpty (feel free to choose a better name) that can be implemented without allocating a set and actually calculating set difference.

193 ↗(On Diff #37702)

Diagnostic messages are not complete sentences, so they don't need periods and capitalization:

... as const reference; consider ...
alexfh requested changes to this revision.Nov 5 2015, 2:01 PM
alexfh edited edge metadata.
This revision now requires changes to proceed.Nov 5 2015, 2:01 PM
alexfh removed a reviewer: klimek.Nov 5 2015, 2:01 PM
flx updated this revision to Diff 43233.Dec 18 2015, 8:34 AM
flx edited edge metadata.
flx marked 7 inline comments as done.

I think, we can arrange a better place for this check. Could you add a new module performance and put this check there?

Thanks!

clang-tidy/misc/ForRangeCopyCheck.cpp
49 ↗(On Diff #43233)

nit: No braces needed here.

63 ↗(On Diff #43233)

nit: No braces needed here.

96 ↗(On Diff #43233)

While I appreciate your effort to bring more functional style to C++ code, this is not always optimal ;)

You only use insertAll together with extractNodesById. It looks like you could instead change the interface of the latter to accept an output parameter (void extractNodesByIdTo(const SmallVector<BoundNodes, 1> &Matches, StringRef ID, set<const Node *> &Nodes)). The result would be fewer allocations of temporary sets.

137 ↗(On Diff #43233)

s/VarDecl/auto/ for consistency with the line below.

150 ↗(On Diff #43233)

nit: If the first if has braces, it's else/else if branch should use braces too.

156 ↗(On Diff #43233)

Please change the punctuation here: "... not a reference type; this ...".

flx updated this revision to Diff 43739.Dec 29 2015, 9:04 AM
flx edited edge metadata.
flx marked 6 inline comments as done.

I'm planning to first submit both http://reviews.llvm.org/D13845 (new argument/param matcher required for this patch) and http://reviews.llvm.org/D15623 (unnecessary copy initialization which introduces the "performance" module).

Then I can update this patch and move the check over to the performance module as well.

flx updated this revision to Diff 43778.Dec 30 2015, 3:14 AM

I moved the ForRangeCopyCheck into the performance module and removed the getPreviousNonCommentToken() function in favor of calling lexer_utils::getPreviousNonCommentToken().

flx updated this revision to Diff 43799.Dec 30 2015, 12:18 PM

I updated this patch to also look at the first argument (the implicit object argument) of const method CxxOperatorCallExprs and added tests to ensure that const member operator invocations as well as non-const member operator invocations are correctly detected.

flx updated this revision to Diff 45338.Jan 19 2016, 8:00 PM

Updated patch against latest head now that the forEachArgumentWithParam matcher is in this patch is not blocked on any external dependencies.

Latest change: I made the LoopVar matcher in registerMatchers() more narrow by not allowing the loop var to be initialized by a MaterializeTemporaryExpr which means that a conversion has already taken place and a const& would not make the loop more performant.

I also added test cases with implicit constructor conversion and user-defined conversion operator to test for this.

flx updated this revision to Diff 45341.Jan 19 2016, 8:36 PM

One more update: Disable the check inside of macros since we cannot place the fixes correctly.

flx updated this revision to Diff 45485.Jan 20 2016, 7:50 PM

Added test to ensure typedef typename T::const_reference does not trigger false positives.

flx updated this revision to Diff 45521.Jan 21 2016, 6:12 AM

The test for const reference typedefs was not complete since the loop variable was not accessed in a const manner in the loop. After I changed that the LoopVar matcher had to be adapted to use hasCanonicalType instead of qualType().

Thank you for the update and sorry for the long delay. Mostly looks good, but you should add user-facing documentation. See docs/clang-tidy/checks/misc-unused-raii.rst for a good example.

clang-tidy/performance/ForRangeCopyCheck.h
20 ↗(On Diff #45521)

Please add a link to the user-facing documentation. In general, we should use the template suggested by add_new_check.py:

/// A short description of the check.
///
/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/<check-name>.htm
32 ↗(On Diff #45521)

nit: I'd insert a linebreak before the comment.

alexfh accepted this revision.Jan 21 2016, 7:15 AM
alexfh edited edge metadata.

Looks good modulo comments (documentation can be added as a follow-up).

Thank you for this awesome check!

clang-tidy/performance/ForRangeCopyCheck.cpp
34 ↗(On Diff #45521)

An ArrayRef<BoundNodes> should be used here instead. Sorry for missing this initially and giving you a sub-optimal suggestion.

35 ↗(On Diff #45521)

A SmallPtrSet<const Node*, ...> might be a better choice here depending on the usual number of nodes in the set. Anyway, this can wait until this shows up in a profile.

This revision is now accepted and ready to land.Jan 21 2016, 7:15 AM
flx updated this revision to Diff 45769.Jan 22 2016, 4:48 PM
flx edited edge metadata.
flx marked 4 inline comments as done.
alexfh added inline comments.Jan 29 2016, 7:40 AM
docs/clang-tidy/checks/performance-for-range-copy.rst
3 ↗(On Diff #45769)

This needs to be updated.

This revision was automatically updated to reflect the committed changes.