[PATCH] D31757: [clang-tidy] Add a clang-tidy check for possible inefficient vector operations

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:81
+          hasLoopInit(LoopVarInit),
+          hasCondition(binaryOperator(hasOperatorName("<"),
+                                      hasLHS(RefersToLoopVar),
----------------
Perhaps you could support other comparisons, but not attempt to generate a fix-it for them? It seems odd that this would trigger for `<` but not `<=`, but I can see why you'd not want to figure out the reserve call for `!(foo >= 10)`.


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:83
+                                      hasLHS(RefersToLoopVar),
+                                      hasRHS(expr().bind(LoopEndExprName)))),
+          hasIncrement(unaryOperator(hasOperatorName("++"),
----------------
Thinking out loud, but, what happens if the loop end expression has some hidden complexity to it? e.g.,
```
for (int i = 0; i < i + 1; ++i) { // This is a "creative" loop.
  v.push_back(i);
}
```


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:84
+                                      hasRHS(expr().bind(LoopEndExprName)))),
+          hasIncrement(unaryOperator(hasOperatorName("++"),
+                                     hasUnaryOperand(RefersToLoopVar))),
----------------
Can you add a test case for post-increment (all of your tests use pre-increment)? Also, count-down loops seem reasonable to support as well, no?
```
for (int i = 10; i >= 0; --i) {
  v.push_back(i);
}
```


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:86-87
+                                     hasUnaryOperand(RefersToLoopVar))),
+          hasBody(anyOf(compoundStmt(statementCountIs(1), has(PushBackCall)),
+                        PushBackCall)),
+          hasParent(compoundStmt(has(VectorVarDefStmt)).bind(LoopParentName)))
----------------
You should update the documentation to mention that this check only worries about a for loop with a single statement in it. It will be surprising that this code does not trigger the same diagnostic:
```
for (...) {
  std::cout << i;
  v.push_back(i);
}
```


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:95-96
+    const MatchFinder::MatchResult &Result) {
+  if (Result.Context->getDiagnostics().hasUncompilableErrorOccurred())
+    return;
+
----------------
We don't usually add this test in to our check calls; why are you adding it here?


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:98
+
+  const SourceManager *SM = Result.SourceManager;
+  const auto *ForLoop = Result.Nodes.getNodeAs<ForStmt>(LoopCounterName);
----------------
Might as well make this a reference rather than a pointer (simplifies code elsewhere).


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:100-104
+  const auto* PushBackCall =
+      Result.Nodes.getNodeAs<CXXMemberCallExpr>(PushBackCallName);
+  const auto* LoopEndExpr =
+      Result.Nodes.getNodeAs<Expr>(LoopEndExprName);
+  const auto* LoopParent =
----------------
Formatting (I would recommend running clang-format over the patch).


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:109-111
+  auto AllVectorVarRefs = utils::decl_ref_expr::allDeclRefExprs(
+      *VectorVarDecl, *LoopParent, *Result.Context);
+  for (const auto *Ref : AllVectorVarRefs) {
----------------
I'm not certain what types are being used here. Can you turn `AllVectorVarRefs` into something with an explicit type so that I can know what `Ref`'s type is?


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:117
+    //
+    // FIXME: make it more intelligent to identified the pre-allocating
+    // operations before the for loop.
----------------
identified -> identify


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.h:20
+/// Finds possible inefficient `std::vector` operations (e.g. `push_back`) in
+/// for-loops that may cause unnecessary memory reallocations.
+///
----------------
Drop the hyphen in for loops.


================
Comment at: docs/clang-tidy/checks/performance-inefficient-vector-operation.rst:7
+Finds possible inefficient `std::vector` operations (e.g. `push_back`) that may
+cause unnecessary memory reallocations.
+
----------------
The docs should talk more about the limitations of the check (like how many statements it can contain, etc).


https://reviews.llvm.org/D31757