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:
> > > > 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?
https://reviews.llvm.org/D31757
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
