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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
