Closed AnthonyVH closed 3 years ago
It seems like GCC isn't too happy with my code changes and neither are some clang versions. I did all tests locally using clang 10.0.0-4. I'll look into the issue and fix it.
Hi @AnthonyVH Thanks for your effort and the explanation of what you did, a welcome improvement, great work!
I'd like to have a second look tomorrow.
The only review result now are:
#if variant_CPP11_OR_GREATER
{%- for n in range(NumParams) %}
template < variant_index_tag_t( {{n}} ) = variant_index_tag( {{n}} )
variant_REQUIRES_B(detail::typelist_type_is_unique< variant_types, {{n}} >::value) >
variant( T{{n}} const & t{{n}} ) : type_index( {{n}} ) { new( ptr() ) T{{n}}( t{{n}} ); }
{% endfor %}
#else
{%- for n in range(NumParams) %}
variant( T{{n}} const & t{{n}} ) : type_index( {{n}} ) { new( ptr() ) T{{n}}( t{{n}} ); }
{% endfor %}
#endif
instead of:
{%- for n in range(NumParams) %}
#if variant_CPP11_OR_GREATER
template < variant_index_tag_t( {{n}} ) = variant_index_tag( {{n}} )
variant_REQUIRES_B(detail::typelist_type_is_unique< variant_types, {{n}} >::value) >
#endif
variant( T{{n}} const & t{{n}} ) : type_index( {{n}} ) { new( ptr() ) T{{n}}( t{{n}} ); }
{% endfor %}
Hi @AnthonyVH Did not find any other thing to mention on my second look. Very happy with your work!
Shall I merge it as-is or do you first want to make further changes yourself?
If you can wait a little longer, I'll go over it tonight to make the white space and layout changes you mentioned.
This branch contains changes required to support multiple identical types (e.g.
nonstd::variant<nonstd::monostate, int, int>
), and accompanying tests. I've tried keeping the code style of my changes identical to what was already there.Note that I couldn't get this working for C++98. The reason being that there's no support for non-type template parameter default arguments for function templates. I use a defaulted function reference template argument to disambiguate between otherwise identical functions, such as constructors for
T1
andT2
for a typenonstd::variant<nonstd::monostate, int, int>
.For the regular constructors (as well as copy constructor) this lack of support for default template arguments can be worked around by disambiguating with a defaulted function argument instead. Unfortunately, this is not possible for the assignment operator, which can only have a single argument.
Hence the lack of C++98 support. Maybe there's another way to do this, but I couldn't think of any. Just in case there's a simple way to add this support, I've already written all required SFINAE helper structs in a C++98-compatible way.
I've also made a couple of changes/fixes to the ordering of some defines, as well as to the tests. It seems that by default
std::variant
was never pulled in, even if C++17 was available. That's now fixed, as well some test code that started failing as a result of this (see individual commits). The tests for all C++ versions now compile and pass.If you don't like having the "multiple identical types" + "test fixes" changes in a single pull request, just let me know. Then I'll try and split them up properly.
Feel free to let me know if there's anything that I overlooked or that I should have done differently!