This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement P0608R3 - A sane variant converting constructor
ClosedPublic

Authored by lichray on Mar 24 2018, 1:24 AM.

Event Timeline

lichray created this revision.Mar 24 2018, 1:24 AM
lichray updated this revision to Diff 139704.Mar 24 2018, 2:16 AM

Use less traits

Has this paper been adopted into the standard yet?

include/variant
1097

I see no need to stop using operator() in favor of a static function. Is there a reason?

Also this would have to be __test.

1109

I would like to see a version of this patch that doesn't use preprocessor expansion to specialize __overload multiple times.

Perhaps adding a bool = is_same<remove_cv_t<First>, bool> to __overload and then specializing once on that?

EricWF added inline comments.Mar 26 2018, 7:49 PM
include/variant
1109

Or perhaps SFINAE-ing the test member instead of specializing __overload.

Something like this maybe?

template <class _Tp, class... _Types>
struct __overload< _Tp, _Types...> : __overload<_Types...>
{
    using __overload<_Types...>::operator();

    template <bool _EnableBool>
    using _DepIsBool = typename __dependent_type<
        enable_if<is_same_v<remove_cv_t<_Tp>, bool> == _EnableBool>, _EnableBool
    >::type;

    template <class _Up, bool _EnableBool = false, class = _DepIsBool<_EnableBool>>
    auto operator()(_Tp, _Up&& __t) const
      -> decltype(_Tp{__t}, __identity<_Tp>());

    template <class _Up, class _Ap = __uncvref_t<_Up>,
        bool _EnableIfBool = true, class = _DepIsBool<_EnableIfBool>>
    auto operator()(bool, _Up&&, int _Ap::*) const
        -> __identity<_Tp>;

    template <class _Up, class _Ap = __uncvref_t<_Up>,
        bool _EnableIfBool = true, class = _DepIsBool<_EnableIfBool>>
    auto operator()(bool, _Up&&) const
        -> enable_if_t<is_same_v<_Ap, bool>, __identity<_Tp>>;
};
lichray marked an inline comment as done.Mar 26 2018, 8:30 PM

Has this paper been adopted into the standard yet?

No. However, it cleanly passed LEWG for recommending a DR against C++17, so I think we can apply it early, just like we sometimes make changes before filing issues :)

include/variant
1109

If we have concepts we can do so, but __overload is already variadic, leaving no space for sfinae...

lichray updated this revision to Diff 139882.Mar 26 2018, 8:31 PM

Keep changes small

lichray added inline comments.Mar 26 2018, 8:34 PM
include/variant
1109

Instantiating function templates and sfinae are the slowest operations in meta programming, and I don't want to do this for all alternatives just because of a rare bool.

EricWF added inline comments.Mar 26 2018, 8:34 PM
include/variant
1109

Hmm. That doesn't seem to quite work..

EricWF added inline comments.Mar 26 2018, 8:49 PM
include/variant
1109

How would you go about testing this?

lichray added inline comments.Mar 26 2018, 9:58 PM
include/variant
1109

I fixed the int _Ap::* -> int _Ap::* = 0 part, but it still doesn't work

[...]test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp:190:37: error: no viable conversion from 'bool' to 'std::variant<std::string, bool>' (aka 'variant<basic_string<char, char_traits<char>, allocator<char> >, bool>')
    std::variant<std::string, bool> v = true;
                                    ^   ~~~~
lichray updated this revision to Diff 140029.Mar 27 2018, 6:11 PM

Macro-free

lichray marked an inline comment as done.Mar 27 2018, 6:11 PM

This LGTM.

However, we should add tests for const bool and volatile bool before committing.

Also I would like @mclow.lists input about applying this DR early since LWG
hasn't commented on it yet.

lichray updated this revision to Diff 140188.Mar 28 2018, 9:50 PM

More tests

lichray retitled this revision from [libc++] Implement P0608R1 - A sane variant converting constructor to [libc++] Implement P0608R2 - A sane variant converting constructor.Jun 4 2018, 12:00 PM
lichray edited the summary of this revision. (Show Details)
lichray updated this revision to Diff 149824.Jun 4 2018, 12:04 PM

Preserve value category in narrowing test.

lichray updated this revision to Diff 149825.Jun 4 2018, 12:09 PM

Refine coding style

lichray updated this revision to Diff 172514.Nov 3 2018, 11:54 PM

Updated implementation for R3

lichray retitled this revision from [libc++] Implement P0608R2 - A sane variant converting constructor to [libc++] Implement P0608R3 - A sane variant converting constructor.Nov 3 2018, 11:57 PM
lichray edited the summary of this revision. (Show Details)

Ping @mclow.lists @EricWF ; the patch still applies, is there any other thing I need to address?

lichray updated this revision to Diff 203733.Jun 9 2019, 5:26 AM

Trivial rebase

A few minor things:

  • the spacing is different from the rest of libc++ (a lot of it was that way before).
  • since this is a paper it should probably be guarded with #if _LIBCPP_STD_VER > 17
  • update the status in www.

Also, the standard says explicitly, "A variant is permitted to hold the same type more than once, and to hold differently cv-qualified versions of the same type." But currently, that is not allowed — not an issue with this patch, but something that should be fixed.

include/variant
1110

Is this structure 100% necessary? Couldn't __overload add an overload to take care of bools? Maybe something like this:

template<class _Up>
auto operator()(_Tp, _Up&&) const
  ->  enable_if_t<
          is_same_v<__uncvref_t<_Up>, bool>,
          __identity<_Tp>
      >;

Ping @mclow.lists @EricWF ; the patch still applies, is there any other thing I need to address?

Just a note: P0608R3 was adopted in San Diego.

lichray marked an inline comment as done.Jun 9 2019, 9:30 PM
  • the spacing is different from the rest of libc++ (a lot of it was that way before).

That's not something I can "fix" in this patch :(

  • since this is a paper it should probably be guarded with #if _LIBCPP_STD_VER > 17

Marshall and me agreed that we are going to apply the paper as a "bugfix." We don't want to make the constructor behave differently under different language versions.

  • update the status in www.

Done.

include/variant
1110

I'm trying to reduce the compile time cost of SFINAE by putting the bool handling logic in specializations.

lichray updated this revision to Diff 203766.Jun 9 2019, 9:32 PM

Update www

This LGTM.

Also I would like @mclow.lists input about applying this DR early since LWG
hasn't commented on it yet.

Even though LWG didn't vote it out as a DR; that's what it is, and I think that we would be doing our users a disservice by not providing this in C++17.

The patch looks fine to me, but I think you should consider making a couple of T.fail.cpp tests, and check to make sure you get the "right error".

test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp
136

If you really want to check that these are "ambiguous" , or "no matching operator=", etc, the way to do that is to define a fail.cpp test, and check the error messages.

EricWF added inline comments.Jun 10 2019, 8:18 PM
include/variant
1128

Do we even support volatile types in variant?

zoecarver added inline comments.Jun 10 2019, 8:22 PM
include/variant
1128

Yes.

All types in Types shall be (possibly cv-qualified) object types that are not arrays.

lichray updated this revision to Diff 203966.Jun 10 2019, 8:30 PM

Add fail tests

lichray marked 2 inline comments as done.Jun 10 2019, 8:38 PM
lichray added inline comments.
test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp
136

(the messages in static_assert are just notes to people who read this code)

I added additional fail.cpp tests.

mclow.lists accepted this revision.Jun 18 2019, 6:44 AM

This looks good to me. Thanks!

This revision is now accepted and ready to land.Jun 18 2019, 6:44 AM
This revision was automatically updated to reflect the committed changes.
lichray marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptJun 18 2019, 8:26 AM