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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__functional_base:343
+template <class _Ret>
+struct __invoke_void_return_wrapper<_Ret, true>
 {
----------------
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.


================
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;
----------------
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?


================
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 {};
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94452