[libcxx-commits] [PATCH] D129229: [libc++] reference_wrapper does not define nested types as described in C++11/14

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__functional/weak_result_type.h:67-68
+#if _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_ENABLE_CXX20_REMOVED_BINDER_TYPEDEFS)
+template <typename...>
+using void_t = void;
+
----------------
You should use `__void_t` instead, it is available in all Standard modes.


================
Comment at: libcxx/include/__functional/weak_result_type.h:70
+
+template <class _Tp, class = void_t<> >
+struct __maybe_has_argument_type {};
----------------



================
Comment at: libcxx/include/__functional/weak_result_type.h:101
+#if _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_ENABLE_CXX20_REMOVED_BINDER_TYPEDEFS)
+    : public __maybe_has_argument_type<_Tp>
+#endif
----------------
Plugging this here means that we also change the behavior of things like `std::mem_fn`. It looks like that's a good change, however we should also test it. Can you please make the fix not only for `std::reference_wrapper` but also for the other things like `std::mem_fn` that seem to be mis-behaving right now?


================
Comment at: libcxx/test/libcxx/utilities/function.objects/refwrap/refwrap_types.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
I would name this test `libcxx/test/libcxx/utilities/function.objects/refwrap/argument_types.compile.pass.cpp` or something like that. But at least let's make it a `.compile.pass.cpp` test since it is compile-only.


================
Comment at: libcxx/test/libcxx/utilities/function.objects/refwrap/refwrap_types.pass.cpp:24
+
+int main(int, char**) {
+  {
----------------
You should make this function just be `void test()` instead of `main` after moving it to `.compile.pass.cpp`. Those tests are never run, only compiled, and not having a `main()` function in a compile-only test helps avoid confusion about whether the test is run.


================
Comment at: libcxx/test/libcxx/utilities/function.objects/refwrap/refwrap_types.pass.cpp:27
+    typedef float(F)(int);
+
+    typedef std::reference_wrapper<F>::argument_type A;
----------------
We should add a test that `reference_wrapper<T>` has no such nested types if `T` isn't a function type or a type with these types nested.

In other words, we want to positively assert that `reference_wrapper<int>` (for example) doesn't provide a meaningless `argument_type` typedef (and same for the other ones).


================
Comment at: libcxx/test/std/depr/depr.function.objects/depr.base/refwrap_types.depr.verify.cpp:9-13
+// <functional>
+// REQUIRES: c++17
+// Types of class reference_wrapper were removed in C++20
+
+// check that types of class reference_wrapper are marked deprecated
----------------
Let's move the `REQUIRES` above `// <functional>` to avoid breaking up the synopsis like that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129229