Diff Detail
- Build Status
Buildable 1451 Build 1451: arc lint + arc unit
Event Timeline
TODO:
- Kill _LIBCPP_NOEXCEPT_SFINAE_RETURN
- Kill _LIBCPP_NOEXCEPT_INIT
- Specify the SFINAE / noexcept conditions at the variant level.
- Update unit tests.
- Killed _LIBCPP_NOEXCEPT_SFINAE_RETURN.
- Killed _LIBCPP_NOEXCEPT_INIT.
- Specified the SFINAE / noexcept conditions at the variant level.
include/variant | ||
---|---|---|
279 | Added it back, since I removed it prematurely. |
- Fixed <variant> comparison operators as per P0393.
- Updated variant synopsis.
- Removed allocator support from variant.
- Removed support for empty set of alternatives in a variant.
- Removed support for void as an alternative in a variant.
- Removed support for reference types in a variant.
- Removed __all_v.
- Moved bad_variant_access to unversioned namespace.
- Removed unnecessary uses of __identity.
- Removed unnecessary use of __size_constant.
- Addressed Eric's comments.
- Updated variant tests.
- Adopted to use the new in_place tags.
- variant_alt() should be = default.
- Fixed variant comparison operators test.
- Removed variant's uses_allocator test.
- Fixed <variant> comparison operators as per P0393.
- Updated variant synopsis.
- Removed allocator support from variant.
- Removed support for empty set of alternatives in a variant.
- Removed support for void as an alternative in a variant.
- Removed support for reference types in a variant.
- Removed __all_v.
- Moved bad_variant_access to unversioned namespace.
- Removed unnecessary uses of __identity.
- Removed unnecessary use of __size_constant.
- Addressed Eric's comments.
- Updated variant tests.
- Adopted to use the new in_place tags.
- variant_alt() should be = default.
- Fixed variant comparison operators test.
- Removed variant's uses_allocator test.
- Disallowed array types in variant.
- Added new tests.
test/std/utilities/variant/variant.variant/variant.mod/emplace_index_init_list_args.pass.cpp | ||
---|---|---|
55 | This test is currently fails. However, TestTypes::NoCtors inherits from ::ArchetypeBases::TestBase<NoCtors> which defines the following constructors: template <bool Dummy = true, typename std::enable_if<Dummy && Explicit, bool>::type = true> explicit TestBase(std::initializer_list<int>& il, int = 0) noexcept : value(il.size()) { ++alive; ++constructed; ++value_constructed; } template <bool Dummy = true, typename std::enable_if<Dummy && !Explicit, bool>::type = true> explicit TestBase(std::initializer_list<int>& il, int = 0) noexcept : value(il.size()) { ++alive; ++constructed; ++value_constructed; } These constructors are not = deleted in NoCtors. So the behavior we observe here is correct. But I'm not sure which direction we should be solving this, since I don't fully understand the intended semantics of these test classes. | |
test/std/utilities/variant/variant.variant/variant.swap/swap.pass.cpp | ||
459–461 | I don't think the non-member swap is SFINAE'd out based on the current wording. | |
537–539 | I don't think the non-member swap is SFINAE'd out based on the current wording. |
include/variant | ||
---|---|---|
274 | Please lift this inside the template body, and add a static assert that the index is in range. | |
334 | __variant_best_match_t if it gives you the type back directly please. | |
335 | In order to avoid forming a function which takes an abstract class by value please change this to. typename result_of_t<__variant_overload<_Types...>(_Tp&&)>::type; | |
551 | Please use _UpperCamelCase for template parameters. | |
765 | Names like __is_nothrow_move_constructible may be the compiler builtin used to implement is_nothrow_move_constructible. Try to avoid them. | |
802 | __assign_alt needs to be public in order to use it in the lambda according to GCC. | |
852 | Seems like an awkward way to make the type dependent. Maybe something like this? bool _Dummy = true, class = enable_if_t< is_default_constructible_v< typename __dependent_type<variant_alternative_t<0, variant>, _Dummy>::type >> | |
864 | The && is almost always unnecessary. Libc++ usually omits it and you should too. | |
867 | _Arg&& __arg (ie add a space) | |
1015 | Clang and GCC disagree on the type of this expression. Clang produces the reference qualified type but GCC does not. I think a better formulation is: return ((void)__holds_alternative<_Ip>(__v) ? (void)0 : throw bad_variant_access{}), __variant_access::__get_alt<_Ip>(_VSTD::forward<_Vp>(__v)).__value; |
Addressed a few of the comments.
include/variant | ||
---|---|---|
274 | Which of the following would you prefer? template <size_t _Ip, class... _Types> struct _LIBCPP_TYPE_VIS_ONLY variant_alternative<_Ip, variant<_Types...>> : __identity<__type_pack_element<_Ip, _Types...>> { static_assert(_Ip < sizeof...(_Types)); }; template <size_t _Ip, class... _Types> struct _LIBCPP_TYPE_VIS_ONLY variant_alternative<_Ip, variant<_Types...>> { static_assert(_Ip < sizeof...(_Types)); using type = __type_pack_element<_Ip, _Types...>; }; |
I think the only thing left to mention are visibility macros.
_LIBCPP_INLINE_VISIBILITY should be applied to most publicly visible functions, especially if they are only a line or two.
Also inline should be added to any non-inline function template. (Note: inline definitions and constexpr functions are both implicitly inline).
include/variant | ||
---|---|---|
544 | This should be constrained to not pick up __variant_alt | |
779 | Why not forward? |
include/variant | ||
---|---|---|
615 | I think we get a better layout putting __index second when one of the types in the variant requires more alignment than size_t. In the current layout that will cause 8 bytes of padding between __index and __data. What do you think? |
include/variant | ||
---|---|---|
779 | haha, had done this myself just in time. |
include/variant | ||
---|---|---|
802 | Updated to this->__assign_alt(...); |
include/variant | ||
---|---|---|
544 | Good catch! I've added an in_place_t tag parameter to avoid this rather than adding a bunch of SFINAE conditions. |
include/variant | ||
---|---|---|
274 | Used the latter. |
TODO:
- Think about layout
- Implement constexpr copy/move ctors for variant that holds all trivially_copyable types.
- Don't forget about https://reviews.llvm.org/D23263#603887
- Investigate https://reviews.llvm.org/D26899#602539
This test fails to compile. Shouldn’t the visit work similarly to invoke?
#include <variant>
#include <functional>
#include <string>
using std::variant;
using std::visit;
struct visitor
{
bool operator () (bool, bool) { return true; } bool operator () (const std::string&, const std::string&) { return true; }
};
int main()
{
visitor vis; variant<bool,std::string> var1, var2; bool b1, b2; std::string str1, str2; b1 = b2 = true; std::invoke(vis, b1, b2); var1 = var2 = true; visit(vis, var1, var2); str1 = str2 = "test"; std::invoke(vis, str1, str2); var1 = var2 = "test"; visit(vis, var1, var2);
}
No, I don't believe so. IIUC, visit requires exhaustive matching from a visitor.
In your example, visitor does not have handlers for (bool, const std::string&) and (const std::string&, bool).
Boost had a similar example. There visit seemed to work similarly to the std::invoke. But maybe that is not required from the std::visit in the standard.
- Added visibility macro _LIBCPP_INLINE_VISIBILITY + inline.
- Preserved triviality of special member functions.
- Added libcxx tests for the trivial special member functions feature.
- Reorganized the structure of the file.
- Consistently used const T rather than T const in tests.
All of the examples on that page perform visitation with a single variant, which satisfies the exhaustive match requirement.
For your multi-visitation example to be exhaustive, you need to handle the bool, const std::string&, and const std::string&, bool
cases. You can achieve that either by adding operator()(bool, const std::string&) and operator()(const std::string&, bool), or
by adding a "default handler", i.e. template <typename T, typename U> operator()(const T&, const U&);.
I see your point now. I guess this is what the "for all combinations of alternative types of all variants" means. I didn't realise it before. Thanks!
- Used unsigned int rather than size_t for index
- Placed index second so that we have trailing padding rather than in the middle.
- Used __sfinae_ctor_base and` __sfinae_assign_base` for special member SFINAE.
LGTM.
I've attached a couple of inline comments that I generated a couple of days ago.
include/variant | ||
---|---|---|
285 | This is beautiful metaprogramming. C++14 constexpr FTW. | |
814 | I really dislike passing uninitialized memory as __variant_alt<_Ip, _Tp>&. I'm pretty sure it's UB. Optimally we should pass this in as a void* but IDK how to easily change this. You don't need to change this. Just leaving a note. | |
816 | Needs (void*) cast to avoid ADL. | |
1216 | I think we should choose a less common value to hash valueless variants to. | |
1232 | I think we want to return a more interesting hash value than that. 0 seems likely to cause hash collisions. Maybe some large prime? |
Woot! Thanks for all your hard work! I'm sure there will be more <variant> changes and cleanup to come, but this is ready to land in-tree.
If you want to use macros like this please define it immediately before the first use, and undefine it immediately after.