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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM once CI passes (after you silence the GCC warning). Ship it once it does!



================
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:
> > 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.
Thanks for confirming. Indeed, it was 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 {};
----------------
Quuxplusone wrote:
> 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.)
Nobody has suggested such a macro yet.


================
Comment at: libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/noncopyable_return_type.pass.cpp:20
+#if defined(__clang__)
+#pragma clang diagnostic ignored "-Wignored-qualifiers"
+#endif
----------------
It looks like you'll need to silence this for GCC too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94452