This is an archive of the discontinued LLVM Phabricator instance.

[libc++] experimental variant class for review
Needs ReviewPublic

Authored by ruoka on Nov 20 2016, 1:52 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

Th libc++ SVN trunk is still seems to be missing the variant class. It would be great to have it included. Here's a my implementation of the variant class and test cases in case libc++ needs one and in case you want to use mine. I believe I did write it libc++ compatible way. Cheers!

Patch by Kaius Ruokonen.

Diff Detail

Event Timeline

ruoka updated this revision to Diff 78665.Nov 20 2016, 1:52 PM
ruoka retitled this revision from to [libc++] experimental variant class for review.
ruoka updated this object.
ruoka added a reviewer: EricWF.
EricWF edited edge metadata.Nov 20 2016, 2:16 PM

We already have a <variant> implementation in the pipeline (https://reviews.llvm.org/D23263). And I would much rather not have two entirely separate variant implementations in libc++.

ruoka added a comment.Nov 21 2016, 1:34 AM

Hi Eric,
Well, that is kind of a good news. I have been following the status of variant in http://libcxx.llvm.org/cxx1z_status.html and being worried because it isn’t even “In progress”. (I’m using it in one of my libraries) I guess the C++1z implementation the status was just outdated. I agree there is no point of having two variant implementations in libc++. Naturally, it’s your choice what implementation you prefer in the end. Cheers.
Br, Kaius

D23263 doesn’t seem to be completely aligned with the latest standard draft
https://github.com/cplusplus/draft/blob/master/papers/n4606.pdf as it doesn’t support references, void, allocators and is_move_assignable_v returned false when it should had returned true.

//All types in Types... shall be (possibly cv-qualified) object types, (possibly cv-qualified) void, or references. [ Note: Implementations could decide to store references in a wrapper. — end note ] 

template <class... Types, class Alloc> struct uses_allocator<variant<Types...>, Alloc> : true_type { };

allocator-extended constructors
It seems that it will take some more time before it’s available in SVN.

mpark added a subscriber: mpark.Nov 21 2016, 6:31 PM

Hello,

as it doesn’t support references, void, allocators

Support for references, void, and allocators were removed at the latest meeting in Issaquah.

is_move_assignable_v returned false when it should had returned true.

This sounds like it may be a bug. Could you please comment on D23263 and point out the problematic case?

ruoka added a comment.EditedNov 22 2016, 6:11 AM

Hi mpark,

Sorry, my bad.

I didn’t know that void, and allocators were removed.

And the below test failed with your implementation but that is what the standard draft required?

#include <variant>

using std::variant;

struct foo
{

// foo(foo&&) {};
foo& operator = (foo&&) noexcept {return *this;};

};

int main()
{

// static_assert(std::is_move_constructible_v<foo>);
static_assert(std::is_move_assignable_v<foo>);
static_assert(std::is_move_assignable_v<variant<foo>>);

}

“Remarks: This function shall not participate in overload resolution unless is_move_constructible_v<Ti> && is_move_assignable_v<Ti> is true for all i.”

I didn’t check the is_move_constructible in my own implementation.

Let’s hope that the variant ships soon with LIBCPP!

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

Hi mpark,

Sorry, my bad.

I didn’t know that void, and allocators were removed.

And the below test failed with your implementation but that is what the standard draft required?

#include <variant>

using std::variant;

struct foo
{
  // foo(foo&&) {};
  foo& operator = (foo&&) noexcept {return *this;};
};

int main()
{
  // static_assert(std::is_move_constructible_v<foo>);
  static_assert(std::is_move_assignable_v<foo>);
  static_assert(std::is_move_assignable_v<variant<foo>>);
}

“Remarks: This function shall not participate in overload resolution unless is_move_constructible_v<Ti> && is_move_assignable_v<Ti> is true for all i.”

I didn’t check the is_move_constructible in my own implementation.

Let’s hope that the variant ships soon with LIBCPP!

I don't see a problem here. The example (as commented out) should, and does fail.
Since foo's move assignment operator is defined, the move constructor is not.
Therefore is_move_constructible_v<foo> returns false.
As you quoted, variant<foo> is move assignable, if foo is move constructible and move assignable.
Since foo is not move constructible, variant<foo> is not move assignable.

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

Hi mpark,

Sorry, my bad.

I didn’t know that void, and allocators were removed.

And the below test failed with your implementation but that is what the standard draft required?

#include <variant>

using std::variant;

struct foo
{
  // foo(foo&&) {};
  foo& operator = (foo&&) noexcept {return *this;};
};

int main()
{
  // static_assert(std::is_move_constructible_v<foo>);
  static_assert(std::is_move_assignable_v<foo>);
  static_assert(std::is_move_assignable_v<variant<foo>>);
}

“Remarks: This function shall not participate in overload resolution unless is_move_constructible_v<Ti> && is_move_assignable_v<Ti> is true for all i.”

I didn’t check the is_move_constructible in my own implementation.

Let’s hope that the variant ships soon with LIBCPP!

I don't see a problem here. The example (as commented out) should, and does fail.
Since foo's move assignment operator is defined, the move constructor is not.
Therefore is_move_constructible_v<foo> returns false.
As you quoted, variant<foo> is move assignable, if foo is move constructible and move assignable.
Since foo is not move constructible, variant<foo> is not move assignable.

Sorry, I was not clear enough. The issue was indeed in my test cases and there was no problem here.

EricWF resigned from this revision.Dec 5 2016, 1:40 AM
EricWF removed a reviewer: EricWF.

I'm very sorry to do this after all your hard work, but I don't think this patch can proceed.

First , and most importantly, <experimental/variant> isn't part of any standard or technical specification AFAIK. We can't just implement new <experimental/foo> headers without a specification.

Secondly, if <experimental/variant> ever gets added its implementation should mirror`<variant>` as closely as possible.

ruoka added a subscriber: EricWF.Dec 6 2016, 9:45 AM

I'm very sorry to do this after all your hard work, but I don't think this patch can proceed.

First , and most importantly, <experimental/variant> isn't part of any standard or technical specification AFAIK. We can't just implement new <experimental/foo> headers without a specification.

Secondly, if <experimental/variant> ever gets added its implementation should mirror`<variant>` as closely as possible.

Hi Erik,

Yes, I got it already.

My intention was just to progress the class varian in libcpp as it wasn’t yet included in svn back then.

Great that you managed to include it there now. Good work!

Br, Kaius