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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:22
+void InefficientVectorOperationCheck::registerMatchers(MatchFinder *Finder) {
+  const auto VectorDecl = cxxRecordDecl(hasName("std::vector"));
+  const auto VectorDefaultConstructorCall = cxxConstructExpr(
----------------
aaron.ballman wrote:
> Why does this check only focus on vector? Other containers allow you to preallocate space for their elements or lend themselves to inefficient operations.
> 
> Also, this should be checking `::std::vector` instead.
> Why does this check only focus on vector? Other containers allow you to preallocate space for their elements or lend themselves to inefficient operations.

Only a few std containers (`vector`, `unordered_map`, `unorder_set`) provide the preallocate ability ("reserve") to users. `vector` is the most widely used container with these inefficient operations in the real world. 

Yes, we could extend this check to support other containers, but it will make the check more complicated (in the future, I plan to support for-range loops and for loops with iterators, and use other  elegant fix-it solutions like "range insert" or "range constructor" in some specific cases rather than merely inserting `reserve` statements).  So I think it makes more sense to make this check only focus on `vector`.



================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:32
+  const auto ReserveCall = cxxMemberCallExpr(
+      callee(cxxMethodDecl(hasName("reserve"))), on(hasType(VectorDecl)));
+  const auto PushBackCall =
----------------
aaron.ballman wrote:
> This isn't paying attention to what is being reserved.
Good catch. Added a test for it.


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:46-50
+          hasCondition(binaryOperator(hasOperatorName("<"),
+                                      hasLHS(RefersToLoopVar),
+                                      hasRHS(expr().bind("loop_end_expr")))),
+          hasIncrement(unaryOperator(hasOperatorName("++"),
+                                     hasUnaryOperand(RefersToLoopVar))),
----------------
aaron.ballman wrote:
> These conditions seem rather restrictive -- why should you not get the same issue with a range-based for loop?
Yeah, this is intended. We want to match typical for-loops with counters here.  For-range loop will be supported in the future.


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:53-54
+                        PushBackCall)),
+          hasParent(compoundStmt(unless(has(ReserveCall)),
+                                 has(VectorVarDefStmt))))
+          .bind("for_loop"),
----------------
aaron.ballman wrote:
> I'm really not keen on this. It will catch trivial cases, so there is some utility, but this will quickly fall apart with anything past the trivial case.
The motivation of this check is to find code patterns like `for (int i = 0; i < n; ++i) { v.push_back(i); }` and clean them in our codebase (we have lots of similar cases). [These](https://docs.google.com/document/d/1Bbc-6DlNs6zQujWD5-XOHWbfPJVMG7Z_T27Kv0WcFb4/edit?usp=sharing) are all cases we want to support. Using `hasParent` is a simple and sufficient way to do it IMO.


================
Comment at: test/clang-tidy/performance-inefficient-vector-operation.cpp:53
+    std::vector<int> v;
+    v.reserve(20);
+    // CHECK-FIXES-NOT: v.reserve(10);
----------------
aaron.ballman wrote:
> I'd like to see a test where you reserve 5 elements and then loop for 10 elements.
Even for this case, the check will also ignore it.


https://reviews.llvm.org/D31757