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

[Michael Park via Phabricator via cfe-commits]

mpark added inline comments.


================
Comment at: include/variant:295
+
+template <size_t _NumElem>
+using __variant_index_t =
----------------
`s/_NumElem/_NumAlts/`


================
Comment at: include/variant:300-303
+  std::tuple_element_t<
+      __choose_index_type(_NumElem),
+      std::tuple<unsigned char, unsigned short, unsigned int>
+  >;
----------------
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>;
```


================
Comment at: include/variant:306
+
+template <class _IntType>
+constexpr _IntType __variant_npos = static_cast<_IntType>(-1);
----------------
`s/_IntType/_IndexType/` maybe?


================
Comment at: test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp:17
+
+#include <cassert>
+#include <limits>
----------------
Doesn't seem like this is used?


================
Comment at: test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp:26
+
+template <size_t ...Indexes>
+struct make_variant_imp<std::integer_sequence<size_t, Indexes...>> {
----------------
Hm, I see 15 instances of `Indices` and 7 instances of `Indexes`.
Can we use `Indices` everywhere?


================
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<
----------------
I guess I asked you to remove this above. Is this an essential part of the test?


https://reviews.llvm.org/D40210