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

[Mehdi AMINI via llvm-dev]

On Tue, Nov 17, 2020 at 1:40 PM Sean Silva <chisophugis at gmail.com> wrote:

>
>
> 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?
>

Looks fine! Thanks


>
> -- 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/d51deb54/attachment.html>