[PATCH] D90884: [SmallVector] Add a default small size.

[Sean Silva via Phabricator via llvm-commits]

silvas added inline comments.


================
Comment at: llvm/unittests/ADT/SmallVectorTest.cpp:1013
+    TestDefaultSmallSize<int32_t>, TestDefaultSmallSize<int64_t>,
+    TestDefaultSmallSize<void *>, TestDefaultSmallSize<BigElementType>>;
+
----------------
mehdi_amini wrote:
> silvas wrote:
> > mehdi_amini wrote:
> > > FWIW I am still unconvinced that it is a good idea to use the default size on large objects. I'm not super comfortable with the implicit `0` as that seems to encourage mis-uses of the SmallVector.
> > I don't feel strongly about this, except on the usability side. But maybe that's a minor concern, as very large types are probably not that common.
> > 
> > One real issue is that rejecting SmallSize==0 could result in unexpected static_assert's across platforms: sizeof(header) and sizeof(ValueType) will differ across platforms, and both affect whether the default small size will be 0.
> > 
> > I don't see an obvious solution to that, and it seems like it could cause bot breakages. Thoughts?
> I agree the risk of portability is a concern.
> 
> I'm not convinced that this is the right direction for SmallVector here though: it does not seem to me that there is an obvious universal default that always makes sense here. To me this feel like making an API that is "too easy to misuse".
AFAIK, the main misuse of SmallVector is to make its sizeof too big by not paying attention to sizeof(ValueType) and the number of inline elements. This patch specifically bounds that misuse by using a fixed upper bound. (that is, it seems to make the situation strictly better)

I would categorize "using SmallVector but unexpectedly getting 0 inline elements" as a much more minor kind of misuse.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90884