[libcxx-commits] [PATCH] D54410: [libc++] Add C++17 deduction guides for std::function

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

This patch looks like it's got much more complete unit tests than my D64752 <https://reviews.llvm.org/D64752>. I'd be happy to abandon that one. :)



================
Comment at: libcxx/include/functional:1704
 
+#if _LIBCPP_STD_VER > 14 && !defined(_LIBCPP_HAS_NO_DEDUCTION_GUIDES)
+template<class _Rp, class ..._Ap>
----------------
ldionne wrote:
> mclow.lists wrote:
> > 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
> > 
> 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?
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. :)


================
Comment at: libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/deduct_F.fail.cpp:25
+struct R { };
+struct f0 { R operator()() && { return {}; } };
+
----------------
You might want to additionally test (also UB):

    struct f1 { R operator()(int, ...) { return {}; } };


================
Comment at: libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/deduct_F.fail.cpp:28
+int main() {
+    std::function f = f0{}; // expected-error{{no viable constructor or deduction guide for deduction of template arguments of 'function'}}
+}
----------------
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.


================
Comment at: libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/deduct_F.pass.cpp:49
+DECLARE_FUNCTIONS_WITH_QUALS(14, volatile & noexcept);
+DECLARE_FUNCTIONS_WITH_QUALS(15, const volatile & noexcept);
+
----------------
These macros are either awesome or awful, depending on your point of view. ;)
It might be interesting to add `virtual` as another dimension.


================
Comment at: libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/deduct_F.pass.cpp:56
+    std::function a = f0_##N{};                                               \
+    static_assert(std::is_same_v<decltype(a), std::function<R()>>);           \
+                                                                              \
----------------
This should use `ASSERT_SAME_TYPE(decltype(a), std::function<R()>)` for clarity.


================
Comment at: libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/deduct_ptr.pass.cpp:29
+R f2(A1, A2) { return {}; }
+R f3(A1, A2, A3) { return {}; }
+
----------------
I recommend adding a test case for `R f4(A1 = {})` and making sure it deduces `R(A1)` not `R()`.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54410/new/

https://reviews.llvm.org/D54410