[PATCH] D40210: [libc++] Shrink variant's index type when possible
Eric Fiselier via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sat Nov 18 19:47:29 PST 2017
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
More information about the cfe-commits
mailing list