[review][variant2] Variant2 Review Starts April 1

Michael Caisse

24 Mar 2019 24 Mar '19

9:04 p.m.

The Boost formal review of Peter Dimov's Variant2 library will take place April 1 - 10, 2019. Please consider participating in this review. The success of Boost is partially a result of the quality review process which is conducted by the community. You are part of the Boost community. I will be grateful to receive a review based on whatever level of effort or time you can devote. Variant2 is a never-valueless C++11/14/17 implementation of std::variant.

...

From the README:

The class boost::variant2::variant is an almost conforming implementation of std::variant with the following differences: * A converting constructor from, e.g. variant to variant is provided as an extension; * The reverse operation, going from variant to variant is provided as the member function subset. (This operation can throw if the current state of the variant cannot be represented.) * variant is not trivial when all contained types are trivial. To avoid going into a valueless-by-exception state, this implementation falls back to using double storage unless * one of the alternatives is the type monostate, * one of the alternatives has a nonthrowing default constructor, or * all the contained types are nothrow move constructible. If the first two bullets don't hold, but the third does, the variant uses single storage, but emplace constructs a temporary and moves it into place if the construction of the object can throw. In case this is undesirable, one can force emplace into always constructing in- place by adding monostate as one of the alternatives. You can find the source code here: https://github.com/pdimov/variant2 and the documentation here: https://pdimov.github.io/variant2/doc/html/variant2.html Please take an early look at the library and ask questions now. Peter Dimov is very active on the Cpplang Slack #boost channel; however, please consider using the Boost Dev or User Mail Lists so that the entire community might benefit from your conversation. Note: The repository contains an expected implementation also; however, that is not being considered in this review. Thank you for your participation! michael -- Michael Caisse Ciere Consulting ciere.com

Show replies by date

Andrzej Krzemienski

25 Mar 25 Mar

8:31 a.m.

Hi Michael, Thanks for the announcement ahead of time. I have found a ten-day period always to be too short for doing a review, so I appreciate any opportunity to start earlier. My question to you and peter is what exactly are we going to review. The referenced GitHub repository has at least two branches: master and develop. In case they differ, which is the one we are supposed to be reviewing. In the same vein, is the linked documentation exactly what we are going to review? Documentation is also subject to the review process. Peter mentioned he is going to change the docs so that they reflect how the never-valueless guarantee is implemented. Is that change going to be reflected in the documentation version that is going to be reviewed? Regards, &rzej; niedz., 24 mar 2019 o 22:12 Michael Caisse via Boost napisał(a):

...

The Boost formal review of Peter Dimov's Variant2 library will take place April 1 - 10, 2019.

Please consider participating in this review. The success of Boost is partially a result of the quality review process which is conducted by the community. You are part of the Boost community. I will be grateful to receive a review based on whatever level of effort or time you can devote.

Variant2 is a never-valueless C++11/14/17 implementation of std::variant.

From the README:

The class boost::variant2::variant is an almost conforming implementation of std::variant with the following differences:

* A converting constructor from, e.g. variant to variant is provided as an extension;

* The reverse operation, going from variant to variant is provided as the member function subset. (This operation can throw if the current state of the variant cannot be represented.)

* variant is not trivial when all contained types are trivial.

To avoid going into a valueless-by-exception state, this implementation falls back to using double storage unless

* one of the alternatives is the type monostate,

* one of the alternatives has a nonthrowing default constructor, or

* all the contained types are nothrow move constructible.

If the first two bullets don't hold, but the third does, the variant uses single storage, but emplace constructs a temporary and moves it into place if the construction of the object can throw. In case this is undesirable, one can force emplace into always constructing in- place by adding monostate as one of the alternatives.

You can find the source code here: https://github.com/pdimov/variant2

and the documentation here: https://pdimov.github.io/variant2/doc/html/variant2.html

Please take an early look at the library and ask questions now. Peter Dimov is very active on the Cpplang Slack #boost channel; however, please consider using the Boost Dev or User Mail Lists so that the entire community might benefit from your conversation.

Note: The repository contains an expected implementation also; however, that is not being considered in this review.

Thank you for your participation! michael

-- Michael Caisse Ciere Consulting ciere.com

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

Michael Caisse

3:44 p.m.

On 3/25/19 01:31, Andrzej Krzemienski via Boost wrote:> Hi Michael,

...

niedz., 24 mar 2019 o 22:12 Michael Caisse via Boost napisał(a):

...
The Boost formal review of Peter Dimov's Variant2 library will take place April 1 - 10, 2019.

...

My question to you and peter is what exactly are we going to review. The referenced GitHub repository has at least two branches: master and develop. In case they differ, which is the one we are supposed to be reviewing.

In the same vein, is the linked documentation exactly what we are going to review? Documentation is also subject to the review process. Peter mentioned he is going to change the docs so that they reflect how the never-valueless guarantee is implemented. Is that change going to be reflected in the documentation version that is going to be reviewed?

The mater branch will be stable for the review. The documentation comes from the master branch and will therefore be stable also. Peter is currently making changes to develop and will merge those to master for an April 1st review date. The official start-of-review announcement will contain additional details such as review criteria. Thank you for the questions and the early start on getting familiar with the library. michael -- Michael Caisse Ciere Consulting ciere.com

Larry Evans

6 Apr 6 Apr

5:27 a.m.

New subject: [review][variant2] visit needs better user documentation

On 3/24/19 4:04 PM, Michael Caisse via Boost wrote:

...

The Boost formal review of Peter Dimov's Variant2 library will take place April 1 - 10, 2019.

[snip] The documents here: https://pdimov.github.io/variant2/doc/html/variant2.html#ref_visit suggest visit can take any number of arguments. However, the attached fails to compile when defined(ARGS5). The user docs should make clear that there is a limit to the number of arguments. -regards, Larry

Peter Dimov

1:44 p.m.

New subject: [review][variant2] visit needs better user documentation

Larry Evans wrote:

...

The documents here:

https://pdimov.github.io/variant2/doc/html/variant2.html#ref_visit

suggest visit can take any number of arguments. However, the attached fails to compile when defined(ARGS5). The user docs should make clear that there is a limit to the number of arguments.

The error has nothing to do with number of arguments. The function object needs to be callable with any combination of types; the first variant, for instance, may hold a string, and therefore, the function object must be able to accept a string as its first argument.

Larry Evans

3:01 p.m.

New subject: [review][variant2] visit needs better user documentation

On 4/6/19 8:44 AM, Peter Dimov via Boost wrote:

...

Larry Evans wrote:

...
The documents here:

https://pdimov.github.io/variant2/doc/html/variant2.html#ref_visit

suggest visit can take any number of arguments. However, the attached fails to compile when defined(ARGS5). The user docs should make clear that there is a limit to the number of arguments.

The error has nothing to do with number of arguments. The function object needs to be callable with any combination of types; the first variant, for instance, may hold a string, and therefore, the function object must be able to accept a string as its first argument.

I only drew that conclusion because the number of args seems the only difference between the test that compiles OK and the one that doesn't. The code was copy&pasted from: https://github.com/pdimov/variant2/blob/develop/test/variant_visit.cpp#L105 and the function object there only accepts `int` as it's first argument. So, does that compile because any of the alternative types can be converted to `int`? Hmmm. Changed the all types of the lamba args to auto and it still fails to compile :( Why is that? It would illustrate better how to use `visit` if you provided some examples other than those in the test directory. I'm also having a hard time understanding how the implementation at https://github.com/pdimov/variant2/blob/develop/include/boost/variant2/varia... works. Maybe some in source comments would help there. -regards, Larry

Steven Watanabe

3:20 p.m.

New subject: [review][variant2] visit needs better user documentation

AMDG On 4/6/19 9:01 AM, Larry Evans via Boost wrote:

...

<snip> I only drew that conclusion because the number of args seems the only difference between the test that compiles OK and the one that doesn't. The code was copy&pasted from:

https://github.com/pdimov/variant2/blob/develop/test/variant_visit.cpp#L105

and the function object there only accepts `int` as it's first argument. So, does that compile because any of the alternative types can be converted to `int`?

Yes.

...

Hmmm. Changed the all types of the lamba args to auto and it still fails to compile :( Why is that?

The error is different, though. It fails because the function body doesn't compile with every combination of types. In Christ, Steven Watanabe

Peter Dimov

3:51 p.m.

New subject: [review][variant2] visit needs better user documentation

Larry Evans wrote:

...

I only drew that conclusion because the number of args seems the only difference between the test that compiles OK and the one that doesn't. The code was copy&pasted from:

https://github.com/pdimov/variant2/blob/develop/test/variant_visit.cpp#L105

and the function object there only accepts `int` as it's first argument. So, does that compile because any of the alternative types can be converted to `int`?

Yes.

...

Hmmm. Changed the all types of the lamba args to auto and it still fails to compile :( Why is that?

If we're talking about this program: #include #include <string> using namespace boost::variant2; int main() { variant v1( 1 ); variant const v2( 3.14f ); variant v3( 6.28 ); variant const v4( 'A' ); variant const v5( "xxx" ); visit( []( auto x1, auto x2, auto x3, auto x4, auto x5 ){}, v1, v2, v3, v4, v5 ); } it compiles with g++/clang++ -std=c++14, and doesn't with -std=c++11. It also doesn't compile on VS2017 but compiles on VS2019. That's because the unlimited argument implementation is disabled on C++11 or VS2017 here: https://github.com/pdimov/variant2/blob/develop/include/boost/variant2/varia... and, you're right, the workaround only supports up to four arguments.

Larry Evans

5:20 p.m.

New subject: [review][variant2] visit needs better user documentation

On 4/6/19 10:51 AM, Peter Dimov via Boost wrote:

...

Larry Evans wrote:

...
I only drew that conclusion because the number of args seems the only difference between the test that compiles OK and the one that doesn't. The code was copy&pasted from:

https://github.com/pdimov/variant2/blob/develop/test/variant_visit.cpp#L105

and the function object there only accepts `int` as it's first argument. So, does that compile because any of the alternative types can be converted to `int`?

Yes.

...
Hmmm. Changed the all types of the lamba args to auto and it still fails to compile :( Why is that?

If we're talking about this program:

#include #include <string>

using namespace boost::variant2;

int main() { variant v1( 1 ); variant const v2( 3.14f ); variant v3( 6.28 ); variant const v4( 'A' ); variant const v5( "xxx" );

visit( []( auto x1, auto x2, auto x3, auto x4, auto x5 ){}, v1, v2, v3, v4, v5 ); }

it compiles with g++/clang++ -std=c++14, and doesn't with -std=c++11. It also doesn't compile on VS2017 but compiles on VS2019. That's because the unlimited argument implementation is disabled on C++11 or VS2017 here:

https://github.com/pdimov/variant2/blob/develop/include/boost/variant2/varia...

and, you're right, the workaround only supports up to four arguments.

Thanks for clearing that up. However, I did some more exploring, and with the attached: when only defined(UNSIGNED_I4), it compiles within a few secs. when only defined(UNSIGNED_I5), it times several seconds consuming 100% of one of my cpu's, but eventually compiles. OTOH, when onely UNSIGNED_I5) is defined, it never terminates: --{--cut here-- -*- mode: compilation; default-directory: "~/prog_dev/boost/releases/ro/boost_1_69_0/sandbox/pdimov/variant2/review/" -*- Compilation started at Sat Apr 6 12:13:45 make -k run /usr/bin/clang++ -c -O0 -gdwarf-2 -std=c++17 -ftemplate-backtrace-limit=0 -fdiagnostics-show-template-tree -fno-elide-type -fmacro-backtrace-limit=0 -I../variant2/include -I/home/evansl/prog_dev/boost/releases/ro/boost_1_69_0 -ftemplate-depth=200 visit_unsigned_i.cpp -MMD -o /tmp/build/clangxx6_0_pkg/boost/releases/ro/boost_1_69_0/sandbox/pdimov/variant2/review/visit_unsigned_i.o /home/evansl/prog_dev/root.imk:182: recipe for target '/tmp/build/clangxx6_0_pkg/boost/releases/ro/boost_1_69_0/sandbox/pdimov/variant2/review/visit_unsigned_i.o' failed make: *** [/tmp/build/clangxx6_0_pkg/boost/releases/ro/boost_1_69_0/sandbox/pdimov/variant2/review/visit_unsigned_i.o] Interrupt Compilation interrupt at Sat Apr 6 12:14:33 --}--cut here-- Have you any idea why? -regards, Larry

Larry Evans

5:34 p.m.

New subject: [review][variant2] visit needs better user documentation

On 4/6/19 12:20 PM, Larry Evans via Boost wrote: [snip]

...

However, I did some more exploring, and with the attached:

when only defined(UNSIGNED_I4), it compiles within a few secs. when only defined(UNSIGNED_I5), it times several seconds consuming 100% of one of my cpu's, but eventually compiles. OTOH, when onely UNSIGNED_I5) is defined, it never terminates: [snip] The above should read:

OTOH, when only UNSIGNED_I6 is defined, it never terminates (waited more than 30secs before killing the process).

Peter Dimov

5:43 p.m.

New subject: [review][variant2] visit needs better user documentation

Larry Evans wrote:

...

Thanks for clearing that up.

However, I did some more exploring, and with the attached:

when only defined(UNSIGNED_I4), it compiles within a few secs. when only defined(UNSIGNED_I5), it times several seconds consuming 100% of one of my cpu's, but eventually compiles. OTOH, when onely UNSIGNED_I5) is defined, it never terminates:

4^4 = 256. 5^5 = 3125. 6^6 = 46656.

Larry Evans

8:56 p.m.

New subject: [review][variant2] visit needs better user documentation

On 4/6/19 12:43 PM, Peter Dimov via Boost wrote:

...

Larry Evans wrote:

...
Thanks for clearing that up.

However, I did some more exploring, and with the attached:

when only defined(UNSIGNED_I4), it compiles within a few secs. when only defined(UNSIGNED_I5), it times several seconds consuming 100% of one of my cpu's, but eventually compiles. OTOH, when onely UNSIGNED_I5) is defined, it never terminates:

4^4 = 256. 5^5 = 3125. 6^6 = 46656.

Ah! Thanks. That information would be useful in the user documentation to give future users a heads up about the compile time complexity.

Larry Evans

8:52 p.m.

New subject: [review][variant2] visit needs better user documentation

On 4/6/19 10:01 AM, Larry Evans via Boost wrote: [snip]

...

It would illustrate better how to use `visit` if you provided some examples other than those in the test directory.

Such as the example bracketed by the `#ifdef UNSIGNED_I4` in the attached. [snip]

2061

Age (days ago)

2074

Last active (days ago)

List overview

Download

12 comments

5 participants

participants (5)

Andrzej Krzemienski
Larry Evans
Michael Caisse
Peter Dimov
Steven Watanabe