[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

Fri Apr 24 12:26:44 PDT 2020

dblaikie accepted this revision.
dblaikie added a comment.

In D77621#2001099 <https://reviews.llvm.org/D77621#2001099>, @dexonsmith wrote:

> In D77621#2000237 <https://reviews.llvm.org/D77621#2000237>, @nikic wrote:
>
> > Okay, I'm basically fine with that, if it is our stance that SmallVector should always be preferred over std::vector. Not really related to this revision, but it would probably help to do some renaming/aliasing to facilitate that view. Right now, the number of `SmallVector<T, 0>` uses in LLVM is really small compared to the `std::vector<T>` uses (100 vs 6000 based on a not very accurate grep). I think part of that is in the name, and calling it `using Vector<T> = SmallVector<T, 0>` and `using VectorImpl<T> = SmallVectorImpl<T>` would make it a lot more obvious that this is our preferred general purpose vector type, even if the stored data is not small.
>
>
> Those aliases SGTM.

I'd be slightly against, just because having a name that differs from the standard name only in case seems pretty subtle - that and momentum, we've had SmallVector around for a while & I think it's OK. I don't mind some places using std::vector either, though. Don't feel strongly enough that I'd outright stand against such an alias/change, but just expressing this amount of disfavor.

In D77621#2001378 <https://reviews.llvm.org/D77621#2001378>, @nikic wrote:

> So tl;dr looks like as long as we keep `grow_pod` outside the header file, this change seems to be approximately free in terms of compile-time and memory usage both.

Awesome - thanks for looking into it!

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77621/new/

https://reviews.llvm.org/D77621