[PATCH] D90884: [SmallVector] Add `SVec<T>` / `Vec<T>` convenience wrappers.

Fri Nov 13 19:12:01 PST 2020

mehdi_amini added inline comments.

================
Comment at: llvm/docs/ProgrammersManual.rst:1517
+* ``Vec<T>`` should be used for vectors on the heap. This includes inside other
+  data structures, such as ``DenseMap<Value *, Vec<Value *>>``.
+
----------------
mehdi_amini wrote:
> silvas wrote:
> > mehdi_amini wrote:
> > > silvas wrote:
> > > > mehdi_amini wrote:
> > > > > I don't necessarily subscribe to the stack/heap difference.
> > > > > 
> > > > > If the SVec/Vec is part of a heap allocation, they retain their properties: `Vect` would always an **extra** heap allocation for the data while SVec keeps inlined storage in the original heap allocation (with all the benefits of SmallVector).
> > > > > 
> > > > > I see it similarly to trailing allocation in heap allocated object somehow.
> > > > > In general, heap or stack, SmallVector and Vec behaves differently in "move" vs "copy".
> > > > This is definitely a simplification, and these are just "guidelines". There are definitely good reasons to use SVec on the heap or Vec on the stack. I think this is the safest default advice we can give, especially since large SmallVector on the heap seems to be one of the main patterns that has been historically problematic.
> > > Can you phrase this in a less prescriptive way? "should" comes a bit strong to me. I think it'd be enough to just say:
> > > 
> > > ```
> > > * ``SVec<T>`` should be used for vectors where a "small" number
> > >   of elements are expected. (the "S" in the name stands for "Small" or
> > >   "on the Stack").
> > > * ``Vec<T>`` should be used for cases where inline storage is too expensive. For example inside other
> > >   data structures, such as ``DenseMap<Value *, Vec<Value *>>``.
> > > ```
> > softened "should" to "recommend".
> Yeah but I really prefer that we focus on small and leave out the stack/heap thing: this looks very misleading to me.
> 
(and it does not provide more/useful information either)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90884