This checker is to warn about the performance overhead caused by concatenating strings using the operator+ instead of basic_string's append or operator+=. It is configurable. In strict mode it matches every instance of a supposed inefficient concatenation, in non-strict mode it matches only those which are inside a loop.
|35 ↗||(On Diff #57294)|
you should swap the two parameters
matching 'hasOverloadedOperatorName' is less expensive
|41 ↗||(On Diff #57294)|
this is not indented correctly.
% clang-format -style=file <your-file> -i
|47 ↗||(On Diff #57294)|
space after ","
|54 ↗||(On Diff #57294)|
You forgot a comment here?
Please create a diff with the full context: http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
|84 ↗||(On Diff #57346)|
nit: Warning messages are not full sentences, so they should not start with a capital letter and should not end with a period.
Please also change the comma to a semicolon ;.
|85 ↗||(On Diff #57346)|
std::basic_string::append is correct, but is an unnecessary detail. I'd suggest changing this to string::append or to just append().
|28 ↗||(On Diff #57894)|
|31 ↗||(On Diff #57894)|
This check is only useful for C++. Please add
if (!getLangOpts().CPlusPlus) return;
|84 ↗||(On Diff #57894)|
The root of inefficiency is in unnecessary allocations of temporary strings, so maybe we should note this in the warning message, e.g. "string concatenation results in allocation of unnecessary temporary strings; consider using 'operator+=' or 'string::append()' instead".
|24 ↗||(On Diff #57894)|
Please remove the empty line.
|25 ↗||(On Diff #57894)|
|33 ↗||(On Diff #57894)|
const bool StrictMode;
|15 ↗||(On Diff #57894)|
Please enclose std::string, std::basic_string and other inline code snippets in double backquotes.
|31 ↗||(On Diff #58054)|
clang-format -style=file, please.
|66 ↗||(On Diff #58054)|
Each additional matcher has a certain overhead, so it's usually beneficial to merge matchers, if they share common parts. In this case, matchers can be rearranged like this:
Finder->addMatcher(exprWithCleanups( hasAncestor(anyOf(cxxForRangeStmt(), whileStmt(), forStmt())), anyOf(hasDescendant(AssingOperator), hasDescendant(PlusOperatorMatcher))));
However, the really serious problem with these matchers is the liberal usage of hasDescendant matcher, which traverses the whole subtree. Nested hasDescendant matchers are even worse. Note that the subtree of a statement can be rather large, especially if you consider lambdas. Apart from performance issues, hasDescendant and hasAncestor can yield unexpected results, in particular, when lambdas or local classes are in play. It's always better to use has, if you only need to match direct children and specifically match intermediate nodes, if the descendant you're interested in is always on a fixed depths.
Please try running your check (and maybe a few others for comparison, you can use clang-tidy -enable-check-profile to get the run time) on a few large translation units to estimate its performance and measure the improvement after changing the code.
|6 ↗||(On Diff #58054)|
The problem subheader is the only one here. It doesn't seem like it helps making the document more structured.
|8 ↗||(On Diff #58054)|
s/is to warn/warns/
|20 ↗||(On Diff #58054)|
Please make the code snippets use LLVM formatting for consistency. In particular, add a space after for and remove the line break before the opening brace.
|39 ↗||(On Diff #58054)|
Add a space before the opening brace.
|41 ↗||(On Diff #59661)|
|48 ↗||(On Diff #59661)|
|57 ↗||(On Diff #59661)|
Please avoid unnecessary negation by putting the positive branch first, this will make the logic slightly simpler.
|67 ↗||(On Diff #59661)|
|67 ↗||(On Diff #59661)|
|50 ↗||(On Diff #61893)|
|68 ↗||(On Diff #61893)|
For 1: if hasAncestor(anyOf(cxxForRangeStmt(), ...)) doesn't work, you can try hasAncestor(stmt(anyOf(cxxForRangeStmt(), ...)). In any case, repeated hasAncestor matchers don't make the check faster ;)
For 2: exprWithCleanups and other implicit nodes can be skipped using the ignoringImplicit() matchers. Leave hasAncestor for the cases, where there can actually be arbitrary nodes in the path.
Here you're matching some arbitrary intermediate node (exprWithCleanups) and then you're constraining its parents and its children. This doesn't make much sense and it's rather inefficient. Since you're interested in the operators in the first place, try rearranging the matcher like this:
cxxOperatorCallExpr( anyOf(AssignOperator, PlusOperator), hasAncestor(stmt(anyOf(cxxForRangeStmt(), whileStmt(), forStmt()))))
A few more nits.
|50 ↗||(On Diff #63098)|
The allOf(declRefExpr(x), declRefExpr(y)) construct can be replaced with declRefExpr(x, y).
|54 ↗||(On Diff #63098)|
Indentation is confusing - as though hasDeclaration is an argument of stmt. Is it a result of clang-format?
|63 ↗||(On Diff #63098)|
hasAncestor is potentially more expensive, so it should go last.
A couple of nits.
|51 ↗||(On Diff #65384)|
As noted earlier, there's no need to repeat declRefExpr. This should be:
|25 ↗||(On Diff #65384)|
Please format the file with clang-format -style=llvm.
Sorry for the delay. I almost missed that there was an update to the patch. Please mark addressed comments as "Done". This way it's easier for me to track changes and it's easier for you not to miss review comments.
One more thing that this check lacks is fix-it hints, however, this can be added later.
|26 ↗||(On Diff #65606)|
The formatting is still off (specifically, indentation of public: and private: and the lack of an empty line before private:).