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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the comments.



================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:81
+          hasLoopInit(LoopVarInit),
+          hasCondition(binaryOperator(hasOperatorName("<"),
+                                      hasLHS(RefersToLoopVar),
----------------
aaron.ballman wrote:
> 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)`.
We could support other comparisons in the future. I think it is a good start to just support `<` in the first version as `<` usage happens more frequently than other comparisons.

Added a fixme.


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:83
+                                      hasLHS(RefersToLoopVar),
+                                      hasRHS(expr().bind(LoopEndExprName)))),
+          hasIncrement(unaryOperator(hasOperatorName("++"),
----------------
aaron.ballman wrote:
> 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);
> }
> ```
Good point. It will copy the expr "i+1" to "reserve" parameter. This kind of case should not be existed, but it is safer to filter out this case.


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:95-96
+    const MatchFinder::MatchResult &Result) {
+  if (Result.Context->getDiagnostics().hasUncompilableErrorOccurred())
+    return;
+
----------------
aaron.ballman wrote:
> We don't usually add this test in to our check calls; why are you adding it here?
It will prevent some unexpected things happened when the translation unit fails to compile.

We had a few experiences before. We encountered some misbehavior of some checks (e.g. https://reviews.llvm.org/rL294578) with a non-compilable TU, and then we added this statement to avoid the misbehavior.



https://reviews.llvm.org/D31757