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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/ADT/SmallVector.h:53
+  // The maximum size depends on size_type used.
+  size_t SizeMax() { return size_type(-1ULL); }
 
----------------
dblaikie wrote:
> I'd probably use numeric_limits here & make this static constexpr
Making it a constexpr /variable/ gets a bit more complicated - do you happen to know off-hand whether this requires a separate/out-of-line definition if it's ODR used (ah, seems it does - in C++14 which LLVM Uses, in 17 ("A constexpr specifier used in a function or static member variable (since C++17) declaration implies inline.") it would be OK)

I think it's best not to leave a trap that might catch someone up in the future if SizeMax ends up getting ODR used (eg: in a call to std::less, rather than an explicit op<, etc) & then the lack of definition would lead to linking failure, etc. So best to leave it as a static constexpr function rather than static constexpr variable.


================
Comment at: llvm/lib/Support/SmallVector.cpp:40-47
+// Check that SmallVector with 1 byte elements takes extra space due to using
+// uintptr_r instead of uint32_t for size and capacity.
+static_assert(sizeof(SmallVector<char, 4>) > sizeof(SmallVector<uint32_t, 1>) ||
+                  sizeof(uintptr_t) == sizeof(uint32_t),
+              "1 byte elements should cause larger size and capacity type");
+static_assert(sizeof(SmallVector<uint32_t, 2>) ==
+                  sizeof(SmallVector<int64_t, 1>),
----------------
I think it's probably best to assert the size more like the above (but using zero-sized inline buffer), rather than using relative sizes - seems like it'd be more readable to me at least. Maybe @dexonsmith has a perspective here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77621