Page MenuHomePhabricator

[libc++] Add C++17 deduction guides for std::function
ClosedPublic

Authored by ldionne on Nov 11 2018, 5:33 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

ldionne created this revision.Nov 11 2018, 5:33 PM
ldionne added inline comments.Nov 11 2018, 5:35 PM
libcxx/include/functional
1711–1745 ↗(On Diff #173606)

Do we have something similar elsewhere I could reuse? This is ugly :(

mclow.lists added inline comments.Jan 10 2019, 10:36 AM
libcxx/include/functional
1704 ↗(On Diff #173606)

In the rest of the library we just check for _LIBCPP_HAS_NO_DEDUCTION_GUIDES, trusting that it is set correctly.

#ifndef _LIBCPP_HAS_NO_DEDUCTION_GUIDES
1711–1745 ↗(On Diff #173606)

Can you use __member_pointer_traits_imp (in type_traits)?

ldionne marked 2 inline comments as done.Jan 28 2019, 11:45 AM
ldionne added inline comments.
libcxx/include/functional
1704 ↗(On Diff #173606)

The reason I'm checking whether C++ > 14 here is that in the future it's possible for new deduction guides to be added after C++17, and we'd only want to enable those in C++ > XX. For example, if we added new deduction guides in C++20, we'd want to say:

#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_DEDUCTION_GUIDES

I wrote it that way for consistency with this pattern. WDYT?

1711–1745 ↗(On Diff #173606)

__member_pointer_traits_imp does not handle noexcept, and it handles C-style variadics (it's not clear to me whether this is supported by the deduction guides, see http://eel.is/c++draft/func.wrap#func.con-12).

If it's OK to

  • add noexcept support to __member_pointer_traits_imp (which means that result_of would work with noexcept-qualified functions, and
  • support C-style variadics in the deduction guide for std::function

then we could add support for noexcept to __member_pointer_traits_imp and use the following instead:

template<class _Fp, class _Stripped = typename __member_pointer_traits<decltype(&_Fp::operator())>::_FnType>
function(_Fp) -> function<_Stripped>;

Gentle ping

Ping. We should decide whether we go forward with this patch or https://reviews.llvm.org/D64752

This patch looks like it's got much more complete unit tests than my D64752. I'd be happy to abandon that one. :)

libcxx/include/functional
1704 ↗(On Diff #173606)

BTW and FWIW, I agree with @EricWF's original comment. In the future it's possible for new deduction guides to be added anywhere; std::function is not special. Even if it has special cover-your-ass wording that pretends it's special. :)

libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/deduct_F.fail.cpp
25 ↗(On Diff #173606)

You might want to additionally test (also UB):

struct f1 { R operator()(int, ...) { return {}; } };
28 ↗(On Diff #173606)

FWIW, this line has undefined behavior (or is IFNDR) — the standard doesn't define the behavior when the deduction guide participates in overload resolution but decltype(&T::operator()) is not of the expected form.

Maybe that means this test ought to go into test/libcxx/ instead of test/std/. Maybe we don't care. I think all of the Big 3 implementations do the same thing in this case.

libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/deduct_F.pass.cpp
49 ↗(On Diff #173606)

These macros are either awesome or awful, depending on your point of view. ;)
It might be interesting to add virtual as another dimension.

56 ↗(On Diff #173606)

This should use ASSERT_SAME_TYPE(decltype(a), std::function<R()>) for clarity.

libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/deduct_ptr.pass.cpp
29 ↗(On Diff #173606)

I recommend adding a test case for R f4(A1 = {}) and making sure it deduces R(A1) not R().

ldionne marked 4 inline comments as done.Jul 17 2019, 7:14 AM
ldionne added inline comments.
libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/deduct_F.fail.cpp
28 ↗(On Diff #173606)

I'm not sure I understand what you mean. You mean that because there's no deduction guide handling the rvalue-ref-qualified-call-operator case, it's ill-formed NDR? What I could find is http://eel.is/c++draft/function.objects#func.wrap.func.con-12:

template<class F> function(F) -> function<see below>;

*Remarks*: This deduction guide participates in overload resolution only if &F::operator() is well-formed when treated as an unevaluated operand. In that case, if decltype(&F::operator()) is of the form R(G::*)(A...) cv_opt & noexcept_opt for a class type G, then the deduced type is function<R(A...)>.

So what you're saying is that since we don't say what happens when decltype(&F::operator()) is *not* of the form R(G::*)(A...) cv_opt & noexcept_opt (in this case it's of the form R(G::*)(A...) cv_opt && noexcept_opt), the behaviour's undefined? I think I agree with that, however I think that's a LWG issue right here -- it should instead say something like:

template<class F> function(F) -> function<see below>;

*Remarks*: This deduction guide participates in overload resolution only if &F::operator() is well-formed when treated as an unevaluated operand, and is of the form R(G::*)(A...) cv_opt & noexcept_opt for a class type G. In that case, the deduced type is function<R(A...)>.

This would make the deduction guide NOT participate in overload resolution if it doesn't have the right form, which is effectively how it's implemented right now. And that mandates a compiler error. Do you agree with me?

ldionne updated this revision to Diff 210327.Jul 17 2019, 7:42 AM

Address comments by Arthur.

Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2019, 7:42 AM
Quuxplusone added inline comments.Jul 17 2019, 7:46 AM
libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/deduct_F.fail.cpp
28 ↗(On Diff #173606)

Yes, I agree with your analysis. It's currently UB; it probably merits an LWG issue to make it not-UB; and almost certainly the right resolution (to the paper issue, and for vendors to use in practice until then) is to make the deduction guide not-participate-in-overload-resolution in the offending cases.

To verify the implementation of that resolution, you'd add a test case like this:

struct A : std::function<int(double)> {
    int operator()() &&;
};
auto x = &A::operator();  // OK
std::function f = A();  // OK, guide doesn't participate in overload resolution, so we deduce from the move-constructor instead
static_assert(std::is_same_v<decltype(f), std::function<int(double)>>);

Which, it occurs to me, also exercises the part of the compiler that generates implicit candidates for copy- and move-constructors in the absence of any definition for the primary template. Isn't CTAD fun? 😛

ldionne marked an inline comment as done.Jul 17 2019, 11:30 AM
ldionne added inline comments.
libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/deduct_F.fail.cpp
28 ↗(On Diff #173606)

I filed a LWG issue and CC'd you on the email.

LGTM modulo these last nitpicky comments.

libcxx/include/functional
2345 ↗(On Diff #210327)

Please make this just

#ifndef _LIBCPP_HAS_NO_DEDUCTION_GUIDES
    ...
#endif

to match the other headers.

...Unless you need the extra _LIBCPP_STD_VER > 14 check, to fix some test failure, in which case please mention it in your commit message. (I do observe that __strip_signature requires a compiler with core-language support for noexcept-in-the-type-system, which was new in C++17, so I will happily be convinced by anecdotal evidence that the difference is required. But I don't want the difference between this header and other headers to be completely gratuitous. :))

2353 ↗(On Diff #210327)

Whitespace nit: I'd remove the space between (_Gp::*) (_Ap...). And maybe the space between _Rp (_Gp::*) too, but I have no real preference on that one.

libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/deduct_F.fail.cpp
18 ↗(On Diff #210327)

Mention the LWG issue here, if it gets a number assigned in time?

libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/deduct_ptr.pass.cpp
111 ↗(On Diff #210327)

I think there should additionally be unit tests for:

  • std::function f = nullptr; does not compile
  • std::function<int(int)> g; std::function f = g; deduces int(int)
  • std::function<int(int)> g; std::function f = std::move(g); deduces int(int)
  • std::function<int(int&&)> g; std::function f = g; deduces int(int&&)
  • std::function<int(int&&)> g; std::function f = std::move(g); deduces int(int&&)

These could be in new test files, or shoehorned into the existing ones somehow, I don't care.

ldionne updated this revision to Diff 210629.Jul 18 2019, 10:39 AM
ldionne marked 4 inline comments as done.

Address Arthur's comments

libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/deduct_F.fail.cpp
18 ↗(On Diff #210327)

There's no issue yet. I'll add it when I get a number.

libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/deduct_ptr.pass.cpp
111 ↗(On Diff #210327)

Good catches.

ldionne accepted this revision.Jul 18 2019, 12:46 PM
This revision is now accepted and ready to land.Jul 18 2019, 12:46 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2019, 12:50 PM