Diff Detail
- Build Status
Buildable 1823 Build 1823: arc lint + arc unit
Event Timeline
Sorry for the delay. I've been looking at this tonight and it looks pretty good. I''ll finish up the review in the morning.
The one big issue I've seen is the handling of special member function SFINAE. I think there are some issues here, but I'll double check the standard docs in the morning.
CMakeLists.txt | ||
---|---|---|
304 ↗ | (On Diff #67545) | Please keep libc++ building at C++11. We can't bump it just yet. |
include/variant | ||
254 | If you want to use macros like this please define it immediately before the first use, and undefine it immediately after. | |
259 | Please scrap this macro and all of its usages entirely. If we want to know if a construction doesn't throw then we should use type traits on the user types, not on our base types. | |
270 | libc++ doesn't put exception types in the versioning namespace. Please define this directly in namespace std. See bad_optional_access for an example. | |
272 | There is no need to declare this destructor. | |
279 | _LIBCPP_TYPE_VIS. | |
311 | add_const<...> has the same effect as __identity<add_const_t<...>>. | |
1093 | Please don't write the SFINAE and noexcept in terms of how the base class acts. The Spec defines the SFINAE/noexcept for this functions as:
Please check these conditions are directly as possible, so it's easy to see if they're correct upon inspection. |
Made some progress tonight. Will continue tomorrow.
- 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.
include/variant | ||
---|---|---|
279 | I've removed the forward declaration of variant. Does the definition need _LIBCPP_TYPE_VIS? or is it only for forward declarations? |
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 ↗ | (On Diff #78689) | 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.