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