[llvm-dev] RFC: [SmallVector] Adding SVec<T> and Vec<T> convenience wrappers.

[Sean Silva via llvm-dev]

On Tue, Nov 17, 2020 at 7:26 AM Chris Lattner <clattner at nondot.org> wrote:

> On Nov 13, 2020, at 2:06 PM, Sean Silva via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
> We've pretty happy now with a patch that adds two wrappers around
> SmallVector that make it 1) more convenient to use and 2) will tend to
> mitigate misuse of SmallVector. We think it's ready for wider discussion:
> https://reviews.llvm.org/D90884
>
> SVec<T> is a convenience alias for SmallVector<T, N> with N chosen
> automatically to keep its size under 64 Bytes (that heuristic is easy to
> change though). The recommendation in the patch is to use this "on the
> stack, where a "small" number of elements are expected".
>
>
> Hey Sean,
>
> I agree with other comments that his approach unnecessarily fragments the
> API surface area for a core class.  You’re doing two things here: 1) adding
> a new name for an existing thing, and 2) adding a default.
>
> My major concern and objection is about #1, since the codebase will get to
> be a mishmash of the two.  I’d rather keep the world consistent here and
> have an opinionated “one way to do it”.
>

Thanks Chris. That was my original proposal when I first drafted the patch,
and I'm happy to just add the default.


>
> Thoughts/suggestions:
>  - Adding the default seems very reasonable to me, and I think that 64
> bytes is a good default.  I think you should change the behavior so that
> SmallVector<LargeThing> defaults to a single inline element instead of zero
> though.  Perhaps generate a static_assert when it is crazy large.
>

That would work for me.

Mehdi, would SmallVector<T>, defaulting to a minimum single inline element
with static_assert(sizeof(T) < 512) address your concerns?

-- Sean Silva


>
>  - The name SmallVector has always been confusing, InlinedVector is more
> principled, but is even more verbose for such a common type.  If you think
> it is worth shrinkifying the name SmallVector, then I think the best thing
> to do is *rename* SmallVector (perhaps to IVector?) everywhere in the
> codebase.  I don’t think that introducing a redundant name is a good thing
> (except to smooth the transition).
>
>  - If people are concerned about the default being bad, then you could
> choose to make “SmallVector<T, -1>” be the thing that autosizes.  I tend to
> agree with your viewpoint that the N chosen is arbitrary in almost all
> cases anyway though, so I wouldn’t go this way.
>
>
> Vec<T> is a convenience alias for SmallVector<T, 0>. It lets us get the
> (little-known?) benefits of SmallVector even when it has no inline elements
> (see https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h).
> The recommendation in the patch is to use this when the SmallVector is on
> the heap.
>
>
> These benefits are tiny, I really don’t think it is worth introducing a
> new name for this.  I think this is better handled with a change to
> CodingStandards (saying “don’t use std::vector”) and the programmers manual.
>
> -Chris
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20201117/1cc7351a/attachment.html>