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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits