hokein marked an inline comment as done.
hokein added a comment.
friendly ping @alexfh.
================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:53-54
+ PushBackCall)),
+ hasParent(compoundStmt(unless(has(ReserveCall)),
+ has(VectorVarDefStmt))))
+ .bind("for_loop"),
----------------
aaron.ballman wrote:
> 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!
I improved the way of catching the preallocations before the loop. Could you
take a look again?
https://reviews.llvm.org/D31757
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
