[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.

LGTM modulo these last nitpicky comments.



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


================
Comment at: libcxx/include/functional:2353
+template<class _Rp, class _Gp, class ..._Ap>
+struct __strip_signature<_Rp (_Gp::*) (_Ap...)> { using type = _Rp(_Ap...); };
+template<class _Rp, class _Gp, class ..._Ap>
----------------
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.


================
Comment at: libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/deduct_F.fail.cpp:18
+// The deduction guides for std::function do not handle rvalue-ref qualified
+// call operators and C-style variadics. Make sure we stick to the specification.
+
----------------
Mention the LWG issue here, if it gets a number assigned in time?


================
Comment at: libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/deduct_ptr.pass.cpp:111
+  }
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54410