Page MenuHomePhabricator

[libc++] Implement LWG 2510
ClosedPublic

Authored by ldionne on Jul 23 2019, 11:33 AM.

Details

Summary

LWG2510 makes tag types like allocator_arg_t explicitly default
constructible instead of implicitly default constructible. It also
makes the constructors for std::pair and std::tuple conditionally
explicit based on the explicit-ness of the default constructibility
for the pair/tuple's elements.

Event Timeline

ldionne created this revision.Jul 23 2019, 11:33 AM
ldionne updated this revision to Diff 211348.Jul 23 2019, 12:11 PM

Rebase onto master

ldionne updated this revision to Diff 211534.Jul 24 2019, 9:44 AM

Add deduction guide for tuple<>. Otherwise, Clang seems to be unable to deduce
std::tuple t{} because the deduction guide leads to an explicit specialization.

ldionne marked an inline comment as done.Jul 24 2019, 9:45 AM
ldionne added a subscriber: Quuxplusone.
ldionne added inline comments.
libcxx/include/tuple
975 ↗(On Diff #221571)

@Quuxplusone Any idea why this is needed? Otherwise, I get:

<snip>/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/implicit_deduction_guides.pass.cpp:125:16: error: no viable constructor or deduction guide for deduction of template arguments of 'tuple'
    std::tuple t1{};
               ^
<snip>/libcxx/include/tuple:643:5: note: candidate template ignored: substitution failure [with _Tp = <>, _Dummy = true]: cannot reference member of primary template because deduced class template specialization 'tuple<>' is an explicit specialization
    tuple()
    ^
<snip>/libcxx/include/tuple:650:5: note: candidate template ignored: substitution failure [with _Tp = <>, _Dummy = true]: cannot reference member of primary template because deduced class template specialization 'tuple<>' is an explicit specialization
    tuple()
    ^
<snip>/libcxx/include/tuple:680:5: note: candidate template ignored: substitution failure [with _Tp = <>, _Dummy = true]: cannot reference member of primary template because deduced class template specialization 'tuple<>' is an explicit specialization
    tuple(const _Tp& ... __t) _NOEXCEPT_((__all<is_nothrow_copy_constructible<_Tp>::value...>::value))
    ^
<snip>/libcxx/include/tuple:698:14: note: candidate template ignored: substitution failure [with _Tp = <>, _Dummy = true]: cannot reference member of primary template because deduced class template specialization 'tuple<>' is an explicit specialization
    explicit tuple(const _Tp& ... __t) _NOEXCEPT_((__all<is_nothrow_copy_constructible<_Tp>::value...>::value))
             ^
<snip>/libcxx/include/tuple:762:9: note: candidate template ignored: substitution failure [with _Tp = <>, _Up = <>]: cannot reference member of primary template because deduced class template specialization 'tuple<>' is an explicit specialization
        tuple(_Up&&... __u)
        ^
<snip>/libcxx/include/tuple:795:9: note: candidate template ignored: substitution failure [with _Tp = <>, _Up = <>]: cannot reference member of primary template because deduced class template specialization 'tuple<>' is an explicit specialization
        tuple(_Up&&... __u)
        ^
<snip>/libcxx/include/__tuple:159:52: note: candidate function template not viable: requires 1 argument, but 0 were provided
template <class ..._Tp> class _LIBCPP_TEMPLATE_VIS tuple;
                                                   ^
<snip>/libcxx/include/tuple:653:5: note: candidate function template not viable: requires 1 argument, but 0 were provided
    tuple(tuple const&) = default;
    ^
<snip>/libcxx/include/tuple:654:5: note: candidate function template not viable: requires 1 argument, but 0 were provided
    tuple(tuple&&) = default;
    ^
<snip>/libcxx/include/tuple:664:5: note: candidate function template not viable: requires 2 arguments, but 0 were provided
    tuple(_AllocArgT, _Alloc const& __a)
    ^
<snip>/libcxx/include/tuple:716:7: note: candidate function template not viable: requires at least 2 arguments, but 0 were provided
      tuple(allocator_arg_t, const _Alloc& __a, const _Tp& ... __t)
      ^
<snip>/libcxx/include/tuple:736:7: note: candidate function template not viable: requires at least 2 arguments, but 0 were provided
      tuple(allocator_arg_t, const _Alloc& __a, const _Tp& ... __t)
      ^
<snip>/libcxx/include/tuple:822:9: note: candidate function template not viable: requires at least 2 arguments, but 0 were provided
        tuple(allocator_arg_t, const _Alloc& __a, _Up&&... __u)
        ^
<snip>/libcxx/include/tuple:842:9: note: candidate function template not viable: requires at least 2 arguments, but 0 were provided
        tuple(allocator_arg_t, const _Alloc& __a, _Up&&... __u)
        ^
<snip>/libcxx/include/tuple:852:9: note: candidate function template not viable: requires single argument '__t', but no arguments were provided
        tuple(_Tuple&& __t) _NOEXCEPT_((is_nothrow_constructible<_BaseT, _Tuple>::value))
        ^
<snip>/libcxx/include/tuple:857:9: note: candidate function template not viable: requires single argument '__t', but no arguments were provided
        tuple(const _Tuple& __t) _NOEXCEPT_((is_nothrow_constructible<_BaseT, const _Tuple&>

[...]
ldionne updated this revision to Diff 211551.Jul 24 2019, 10:24 AM

Rebase on top of CTAD for tuple: https://reviews.llvm.org/D65225

Quuxplusone added inline comments.Jul 24 2019, 1:23 PM
libcxx/include/tuple
975 ↗(On Diff #221571)

I got it! :) https://godbolt.org/z/NGrj_6

template<class...> struct Tuple {
    static constexpr bool Example = true;
    template<bool = Example> Tuple();
};
template<> struct Tuple<> {};

Tuple t;
error: no viable constructor or deduction guide for deduction of template arguments of 'Tuple'
Tuple t;
      ^
note: candidate template ignored: substitution failure [with $0 = <>]: cannot reference member of primary template because deduced class template specialization 'Tuple<>' is an explicit specialization
    template<bool = Example> Tuple();
                    ~~~~~~~  ^
note: candidate function template not viable: requires 1 argument, but 0 were provided
template<class...> struct Tuple {
                          ^

Clang, probably rightly — although I'm not sure — considers only implicit deduction guides that are created from the actual constructors of the primary template. So, when it sees a constructor template, it needs to figure out whether that template corresponds to an actual constructor, or a SFINAE'd-away constructor (because if the latter, then it should not make a deduction guide for it). However, it needs to do that SFINAE check without actually instantiating the primary template (because it's simply not allowed to instantiate the primary template when a suitable specialization exists). So, if the SFINAE-space-validity of the constructor template is dependent on any member of the primary template — such as Tuple<Ts...>::Example in this example, or tuple<_Tp...>::_CheckArgsConstructor in libc++ — then the compiler can't safely figure out if the constructor template corresponds to an actual constructor or not, and so the compiler must throw it out. (Clang treats this as a "substitution failure," but intuitively I don't think that's exactly correct. It's more like, the compiler doesn't even know whether substitution is going to fail or not.)

Conclusion: You could solve the original cascade of errors by making all those helper structs (std::tuple<_Tp...>::_CheckArgsConstructor etc) into non-member struct templates (std::_TupleCheckArgsConstructor<_Tp> etc). Adding the deduction guide seems much simpler, and also follows the letter of the standard, which is a good thing.

ldionne marked an inline comment as done.Jul 24 2019, 2:47 PM
ldionne added inline comments.
libcxx/include/tuple
975 ↗(On Diff #221571)

Woah, thanks a lot for the analysis! Interesting, but indeed I think adding the deduction guides is cleaner.

ldionne updated this revision to Diff 214676.Aug 12 2019, 11:33 AM

Rebase on top of master. This doesn't have a dependency anymore.

zoecarver requested changes to this revision.Aug 18 2019, 11:17 AM
zoecarver added a subscriber: zoecarver.
zoecarver added inline comments.
libcxx/include/type_traits
2819 ↗(On Diff #221571)

You probably need an #if _LIBCPP_STD_VER > 03 here.

libcxx/test/std/utilities/memory/allocator.tag/allocator_arg.fail.cpp
12 ↗(On Diff #221571)

// UNSUPPORTED: c++98, c++03

This revision now requires changes to proceed.Aug 18 2019, 11:17 AM
ldionne marked 2 inline comments as done.Aug 19 2019, 8:37 AM
ldionne added inline comments.
libcxx/include/type_traits
2819 ↗(On Diff #221571)

If you're thinking about the use of decltype, it's fine to assume that we have it in C++03 mode too. However, the implicit construction via {} is indeed a C++11 feature that's not supported in C++03, so I'll need to find another way to perform the check.

I think we want to find a way to do the check in C++03 instead of always having __is_implicitly_default_constructible<T>::value == false because that will change the explicit-ness of things like std::pair in C++03 vs C++11, which is a notable change. Also, the intent of a DR is to have it back-applied so it would be nice if we could implement the issue resolution even in C++03.

I'll admit I can't think of any way to implement the check in C++03 though.

libcxx/test/std/utilities/memory/allocator.tag/allocator_arg.fail.cpp
12 ↗(On Diff #221571)

I think we should do our best to support this test (and the similar ones below) in C++03, since this is a DR.

zoecarver added inline comments.Aug 19 2019, 9:30 AM
libcxx/test/std/utilities/memory/allocator.tag/allocator_arg.fail.cpp
12 ↗(On Diff #221571)

That's probably a good idea. See my comment below. There are a few other tests, maybe we should update/enable those too?

21 ↗(On Diff #221571)

If we are keeping this test we should update this function to return std:: allocator_arg_t ().

ldionne updated this revision to Diff 221571.Sep 24 2019, 12:22 PM
ldionne marked an inline comment as done.

Handle C++03 and C++98 gracefully

mclow.lists accepted this revision.Sep 24 2019, 12:36 PM

this looks fine to me now (with a nit).

libcxx/include/type_traits
2823 ↗(On Diff #221571)

Nit: This comment doesn't really match the #ifndef _LIBCPP_CXX03_LANG that starts the block.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 24 2019, 1:20 PM
Closed by commit rL372777: [libc++] Implement LWG 2510 (authored by ldionne, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2019, 1:20 PM
ldionne reopened this revision.Sep 25 2019, 1:45 PM

This was reverted in r372832. I'm waiting to see the resolution of https://bugs.llvm.org/show_bug.cgi?id=43454 before I know what to do.

libcxx/test/std/utilities/memory/allocator.tag/allocator_arg.fail.cpp
21 ↗(On Diff #221571)

The whole point of this test is to exercise the error caused by the non-explicit copy-initialization. I'm not sure I understand your comment.

zoecarver added inline comments.Sep 25 2019, 2:18 PM
libcxx/test/std/utilities/memory/allocator.tag/allocator_arg.fail.cpp
21 ↗(On Diff #221571)

I think I was worried about C++03 mode. Now that this has been updated to be unsupported in C++03, don't worry about it.

ldionne updated this revision to Diff 221951.Sep 26 2019, 7:38 AM

Workaround PR43454, which previously broke the LLVM build

This revision was not accepted when it landed; it landed in state Needs Review.Sep 26 2019, 7:49 AM
Closed by commit rL372983: [libc++] Take 2: Implement LWG 2510 (authored by ldionne, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.