[PATCH] D20196: [clang-tidy] Inefficient string operation

[Alexander Kornienko via cfe-commits]

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