[libcxx-commits] [PATCH] D94452: [libc++] Support immovable return types in std::function.

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__functional_base:343
+template <class _Ret>
+struct __invoke_void_return_wrapper<_Ret, true>
 {
----------------
ldionne wrote:
> Quuxplusone wrote:
> > ldionne wrote:
> > > I don't understand why this change is necessary. Can you explain?
> > This change is severable (if I sever some of the test cases with it); it's not directly related to NonCopyable.
> > 
> > It allows us to support `std::function<const void()>`, because `std::is_void_v<const void> == true` but `std::is_same_v<const void, void> == false`.
> Ahhhh, so that's it. Ok to leave as-is. I guess I don't care strongly either way, someone using `const void` is doing something wrong, but it might be surprising if it doesn't work.
Agreed, it'd be nice if the Standard just forbade `std::function<R(Args...)>` for all cv-qualified `R`. I hope they're fixing this for `function_ref` and `unique_function` (but I haven't been following that closely).


================
Comment at: libcxx/include/functional:2352
+            static const bool value = is_void<_Rp>::value ||
+                __is_core_convertible<typename __invoke_of<_Fp, _ArgTypes...>::type,
+                                      _Rp>::value;
----------------
ldionne wrote:
> Quuxplusone wrote:
> > ldionne wrote:
> > > Can you explain why we can't use `std::is_convertible`? Is it possible that it's simply a bug in how we implement the `__is_convertible` intrinsic?
> > `std::is_convertible_v<mutex, mutex>` is (intentionally) `false`. So we need a different type-trait or intrinsic to express the core-language idea of "//can be implicitly converted//" as defined in http://eel.is/c++draft/conv.general#3 . libc++ might already have such a type-trait; I admit I didn't look super hard before deciding to write this myself. My intent is that if libc++ ever needs this trait anywhere else, they can reuse `__is_core_convertible` — it's not intended to be `std::function`-specific.
> Ok, so do I understand correctly that `std::is_convertible` (which is really just the `__is_convertible_to` builtin) returns true for `std::is_convertible<std::mutex, std::mutex>` because `is_convertible<From, To>` returns whether the following is well-formed (from https://en.cppreference.com/w/cpp/types/is_convertible):
> 
> ```
> To test() { return std::declval<From>(); }
> ```
> 
> Since `std::declval<std::mutex>()` is `std::mutex&&`, it's basically checking whether we can move-construct a mutex, which isn't the case.
> 
> Instead, what we want to check is whether we can initialize the return value with a prvalue of type `std::mutex`, which is true because of copy/move elision in the language. Am I following this correctly, or am I missing something?
You're following correctly. You did make a typo above — you said "returns true for" when you meant "returns false for" — but all the rest of what you wrote clearly indicates that you've got the idea right and it was //just// a typo.


================
Comment at: libcxx/include/type_traits:1680
+struct __is_core_convertible<_Tp, _Up, decltype(
+    static_cast<void(*)(_Up)>(0) ( static_cast<_Tp(*)()>(0)() )
+)> : public true_type {};
----------------
ldionne wrote:
> Quuxplusone wrote:
> > ldionne wrote:
> > > Can't you use `declval` here?
> > No, because `declval<T>()` gives you a `T&&` instead of a `T`, and we need specifically a `T`.
> > I did initially consider adding a helper function `_Tp __core_declval<_Tp>()` that could be reused elsewhere, but then decided that that was pretty silly because:
> > 
> >     __core_declval<_Tp>()
> >     ((_Tp(*)())0)()  // same effect, shorter, no template-instantiation overhead
> > 
> Thanks for the explanation. But `__core_declval<_Tp>()` does read a lot better than `((_Tp(*)())0)()` IMO. Feel free to leave as-is -- hopefully we won't need this very often.
If we did need it often, I would think about adding a macro version `#define _LIBCPP_CORE_DECLVAL(_Tp) (((_Tp(*)())0)())` to eliminate the template instantiations.
(Off-topic but I wonder if anyone's ever proposed a `_LIBCPP_FWD` macro.)


================
Comment at: libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/noncopyable_return_type.pass.cpp:8
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14
----------------
ldionne wrote:
> `// ADDITIONAL_COMPILE_FLAGS: -Wno-ignored-qualifiers`
> 
> Nifty trick. Also please add a short comment explaining why you're turning that warning off.
Actually grepping libcxx/test/ for `Wno` turned up a better-looking and more consistent solution. See the updated diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94452