[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:53-54
+                        PushBackCall)),
+          hasParent(compoundStmt(unless(has(ReserveCall)),
+                                 has(VectorVarDefStmt))))
+          .bind("for_loop"),
----------------
hokein wrote:
> aaron.ballman wrote:
> > hokein wrote:
> > > aaron.ballman wrote:
> > > > hokein wrote:
> > > > > 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.
> > > > I'm not convinced of the utility without implementing this in a more sensitive way. Have you run this across any large code bases and found that it catches issues?
> > > Yeah, the check catches ~2800 cases (regexp shows ~17,000 total usages) in our internal codebase. And all caught cases are what we are interested in. It would catch more if we support for-range loops and iterator-based for-loops. 
> > I wasn't worried about it not triggering often enough, I was worried about it triggering too often because of the lack of sensitivity. If you randomly sample some of those 2800 cases, do they reserve the space in a way that your check isn't catching?
> Ok, I see your concern now, thanks for pointing it out.
> 
> I have read through these caught cases. The results look reasonable. Most cases (> 95%) are what we expected, the code pattern is like `vector<T> v; for (...) { v.push_back(...); }` where the vector definition statement and for-loop statement are consecutive. Another option is to make the check more strict (only detects the consecutive code pattern).
Okay, that sounds like it has utility then. Thank you for clarifying!


https://reviews.llvm.org/D31757