This is an archive of the discontinued LLVM Phabricator instance.

Implemented C++17 <variant>.
ClosedPublic

Authored by mpark on Aug 8 2016, 6:31 AM.

Details

Reviewers
K-ballo
EricWF

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
mpark updated this revision to Diff 78337.Nov 17 2016, 1:41 AM
  • Removed unnecessary use of __size_constant.
mpark added a comment.Nov 17 2016, 1:42 AM

TODO:

  • Kill _LIBCPP_NOEXCEPT_SFINAE_RETURN
  • Kill _LIBCPP_NOEXCEPT_INIT
  • Specify the SFINAE / noexcept conditions at the variant level.
  • Update unit tests.
This comment was removed by mpark.
mpark updated this revision to Diff 78485.Nov 18 2016, 1:31 AM
  • Killed _LIBCPP_NOEXCEPT_SFINAE_RETURN.
  • Killed _LIBCPP_NOEXCEPT_INIT.
  • Specified the SFINAE / noexcept conditions at the variant level.
mpark updated this revision to Diff 78486.Nov 18 2016, 1:32 AM
This comment was removed by mpark.
mpark marked 5 inline comments as done.Nov 18 2016, 1:33 AM
mpark added inline comments.
include/variant
279

Added it back, since I removed it prematurely.

mpark updated this revision to Diff 78682.Nov 20 2016, 5:02 PM
mpark marked 2 inline comments as done.
  • 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.
mpark updated this revision to Diff 78688.Nov 21 2016, 12:26 AM
  • 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.
mpark updated this revision to Diff 78689.Nov 21 2016, 12:38 AM
  • Removed __get_value function which is now unnecessary
mpark added inline comments.Nov 21 2016, 12:48 AM
test/std/utilities/variant/variant.variant/variant.mod/emplace_index_init_list_args.pass.cpp
56

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
460–462

I don't think the non-member swap is SFINAE'd out based on the current wording.

538–540

I don't think the non-member swap is SFINAE'd out based on the current wording.

ruoka added a subscriber: ruoka.Nov 21 2016, 6:25 AM
mpark updated this revision to Diff 79000.Nov 22 2016, 5:00 PM

Removed Eric's tests.

EricWF added inline comments.Nov 22 2016, 8:15 PM
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;
mpark updated this revision to Diff 79021.Nov 22 2016, 9:15 PM

Addressed a few of the comments.

include/variant
274

Which of the following would you prefer?
I think you're looking for the latter?

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...>;
};
mpark updated this revision to Diff 79022.Nov 22 2016, 9:18 PM

Removed test/std/utilities/lit.local.cfg.

mpark updated this revision to Diff 79026.Nov 22 2016, 9:37 PM
mpark marked 4 inline comments as done.

Removed the use of &&.

mpark marked 2 inline comments as done.Nov 22 2016, 9:38 PM

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?

EricWF added inline comments.Nov 22 2016, 10:58 PM
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?

mpark marked an inline comment as done.Nov 22 2016, 11:09 PM
mpark added inline comments.
include/variant
779

haha, had done this myself just in time.

mpark updated this revision to Diff 79028.Nov 22 2016, 11:12 PM
mpark marked 2 inline comments as done.

Address a few more of Eric's comments.

mpark updated this revision to Diff 79029.Nov 22 2016, 11:27 PM
mpark marked an inline comment as done.

Use an if statement rather than ternary operator.

mpark updated this revision to Diff 79030.Nov 22 2016, 11:28 PM
mpark marked an inline comment as done.

Help out GCC.

mpark marked an inline comment as done.Nov 22 2016, 11:28 PM
mpark added inline comments.
include/variant
802

Updated to this->__assign_alt(...);

mpark updated this revision to Diff 79031.Nov 22 2016, 11:40 PM
mpark marked 2 inline comments as done.

Fix variant swap.

mpark updated this revision to Diff 79032.Nov 22 2016, 11:48 PM

Use member swap to define non-member swap.

mpark updated this revision to Diff 79033.Nov 22 2016, 11:59 PM

Made __variant_alt safer for construction.

mpark added inline comments.Nov 23 2016, 12:00 AM
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.

mpark marked 2 inline comments as done.Nov 23 2016, 12:00 AM
mpark added inline comments.Nov 23 2016, 12:09 AM
include/variant
274

Used the latter.

mpark marked 3 inline comments as done.Nov 23 2016, 12:09 AM
mpark updated this revision to Diff 79034.Nov 23 2016, 12:13 AM

Pulled out __first_type.

TODO:

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);

}

mpark added a comment.Nov 26 2016, 7:49 PM

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).

ruoka added a comment.Nov 29 2016, 9:26 AM

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.

http://www.boost.org/doc/libs/1_61_0/doc/html/variant/reference.html#variant.concepts.static-visitor.examples

mpark updated this revision to Diff 79725.Nov 30 2016, 4:33 AM
  • 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.
mpark added a comment.Nov 30 2016, 4:39 AM

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.

http://www.boost.org/doc/libs/1_61_0/doc/html/variant/reference.html#variant.concepts.static-visitor.examples

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&);.

mpark updated this revision to Diff 79726.Nov 30 2016, 4:41 AM

Rebased

ruoka added a comment.Nov 30 2016, 6:46 AM

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.

http://www.boost.org/doc/libs/1_61_0/doc/html/variant/reference.html#variant.concepts.static-visitor.examples

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!

mpark retitled this revision from Implement <variant>. to Implemented C++17 <variant>..Nov 30 2016, 10:09 AM
mpark updated this revision to Diff 79842.Nov 30 2016, 4:56 PM

Fixed formatting.

mpark updated this revision to Diff 80133.Dec 2 2016, 2:24 PM
  • 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.
EricWF added a comment.Dec 2 2016, 3:05 PM

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?

EricWF accepted this revision.Dec 2 2016, 3:05 PM
EricWF edited edge metadata.

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.

This revision is now accepted and ready to land.Dec 2 2016, 3:05 PM
EricWF closed this revision.Dec 2 2016, 3:11 PM

r288547.