Details
Diff Detail
- Repository
- rCXX libc++
- Build Status
Buildable 24870 Build 24869: arc lint + arc unit
Event Timeline
| libcxx/include/functional | ||
|---|---|---|
| 1711–1745 | Do we have something similar elsewhere I could reuse? This is ugly :( | |
| libcxx/include/functional | ||
|---|---|---|
| 1704 | 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 | __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
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>; | |
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 | 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 | You might want to additionally test (also UB): struct f1 { R operator()(int, ...) { return {}; } }; | |
| 28 | 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 | These macros are either awesome or awful, depending on your point of view. ;) | |
| 56 | 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 | I recommend adding a test case for R f4(A1 = {}) and making sure it deduces R(A1) not R(). | |
| libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/deduct_F.fail.cpp | ||
|---|---|---|
| 28 | 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:
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:
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? | |
| libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/deduct_F.fail.cpp | ||
|---|---|---|
| 28 | 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? 😛 | |
| libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/deduct_F.fail.cpp | ||
|---|---|---|
| 28 | I filed a LWG issue and CC'd you on the email. | |
LGTM modulo these last nitpicky comments.
| libcxx/include/functional | ||
|---|---|---|
| 1704 | Please make this just #ifndef _LIBCPP_HAS_NO_DEDUCTION_GUIDES
...
#endifto 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. :)) | |
| 1712 | 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 | ||
| 19 | 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 | ||
| 112 | I think there should additionally be unit tests for:
These could be in new test files, or shoehorned into the existing ones somehow, I don't care. | |
Address Arthur's comments
| libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/deduct_F.fail.cpp | ||
|---|---|---|
| 19 | 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 | ||
| 112 | Good catches. | |
In the rest of the library we just check for _LIBCPP_HAS_NO_DEDUCTION_GUIDES, trusting that it is set correctly.