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