alexfh requested changes to this revision.
This revision now requires changes to proceed.
================
Comment at: clang-tidy/performance/InefficientStringConcatenationCheck.cpp:31
@@ +30,3 @@
+ MatchFinder *Finder) {
+ if (!getLangOpts().CPlusPlus) return;
+
----------------
clang-format -style=file, please.
================
Comment at: clang-tidy/performance/InefficientStringConcatenationCheck.cpp:66
@@ +65,3 @@
+
+ Finder->addMatcher(WholeMatcher, this);
+ Finder->addMatcher(exprWithCleanups(SurroundLoopMatcher,
----------------
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.
================
Comment at:
docs/clang-tidy/checks/performance-inefficient-string-concatenation.rst:6
@@ +5,3 @@
+
+The problem
+-----------
----------------
`The problem` subheader is the only one here. It doesn't seem like it helps
making the document more structured.
================
Comment at:
docs/clang-tidy/checks/performance-inefficient-string-concatenation.rst:8
@@ +7,3 @@
+-----------
+This check is to warn about the performance overhead arising from
concatenating strings, using the ``operator+``, for instance:
+
----------------
s/is to warn/warns/
s/strings,/strings/
================
Comment at:
docs/clang-tidy/checks/performance-inefficient-string-concatenation.rst:20
@@ +19,3 @@
+ std::string a("Foo"), b("Baz");
+ for(int i = 0; i < 20000; ++i)
+ {
----------------
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.
================
Comment at:
docs/clang-tidy/checks/performance-inefficient-string-concatenation.rst:39
@@ +38,3 @@
+
+ void f(const std::string&){}
+ std::string a("Foo"), b("Baz");
----------------
Add a space before the opening brace.
http://reviews.llvm.org/D20196
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
