[PATCH] D40210: [libc++] Shrink variant's index type when possible

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 4 inline comments as done.
EricWF added inline comments.


================
Comment at: include/variant:300-303
+  std::tuple_element_t<
+      __choose_index_type(_NumElem),
+      std::tuple<unsigned char, unsigned short, unsigned int>
+  >;
----------------
mpark wrote:
> Could we inline the whole thing and also avoid using the `tuple` utilities?
> We should also add the `#include <limits>`
> 
> ```
> conditional_t<
>   (_NumElem < numeric_limits<unsigned char>::max()),
>   unsigned char,
>   conditional_t<(_NumElem < numeric_limits<unsigned short>::max()),
>                 unsigned short,
>                 unsigned int>;
> ```
Not sure that's any cleaner or clearer. Plus it makes it harder to test.

Adding `#include <limits>` as suggested.


================
Comment at: test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp:52-53
+  using Lim = std::numeric_limits<IndexType>;
+  static_assert(std::__choose_index_type(Lim::max() -1) !=
+                std::__choose_index_type(Lim::max()), "");
+  static_assert(std::is_same_v<
----------------
mpark wrote:
> I guess I asked you to remove this above. Is this an essential part of the test?
It is a nice part of it. I pushed back on removing the function as noted above. Perhaps this can stay then?


https://reviews.llvm.org/D40210