This change depends on http://reviews.llvm.org/D13845 which adds a new matcher that allows you match arguments and their parameter declarations together.
- rCTE259199: [clang-tidy] ForRangeCopyCheck that warns on and fixes unnecessary copies of…
rL259199: [clang-tidy] ForRangeCopyCheck that warns on and fixes unnecessary copies of…
rG5aebfe2e4ac5: [clang-tidy] ForRangeCopyCheck that warns on and fixes unnecessary copies of…
Thank you for the patch! One of its dependencies (D13845) is not yet in, but you can in the meantime polish this patch.
|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 ...
I think, we can arrange a better place for this check. Could you add a new module performance and put this check there?
|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 ...".
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.
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.
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.
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.
|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.
Looks good modulo comments (documentation can be added as a follow-up).
Thank you for this awesome check!
|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.