[libcxx-commits] [PATCH] D102992: [libcxx][type_traits] remove `std::is_literal_type` and `std::result_of` for C++20

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

LGTM % comments, again. @ldionne should take a look.



================
Comment at: libcxx/include/type_traits:168-169
     template <class T> struct underlying_type;
-    template <class> class result_of; // undefined
-    template <class Fn, class... ArgTypes> class result_of<Fn(ArgTypes...)>;
+    template <class> class result_of; // undefined; Deprecated in C++17; Removed in C++20
+    template <class Fn, class... ArgTypes> class result_of<Fn(ArgTypes...)>; // Deprecated in C++17; Removed in C++20
     template <class Fn, class... ArgTypes> struct invoke_result;  // C++17
----------------
nit: lowercase "deprecated" and "removed" here and lines 236 and 305 below


================
Comment at: libcxx/include/type_traits:3720
 
-template <class _Tp> struct _LIBCPP_TEMPLATE_VIS is_literal_type
+#if _LIBCPP_STD_VER < 20 || defined(_LIBCPP_ENABLE_CXX20_REMOVED_TYPE_TRAITS)
+template <class _Tp> struct _LIBCPP_TEMPLATE_VIS _LIBCPP_DEPRECATED_IN_CXX17 is_literal_type
----------------
Please stick to `_LIBCPP_STD_VER <= 17` rather than `_LIBCPP_STD_VER < 20`, for consistency with the other places we do things like this. (It never really matters once the year has passed, but for example, this year, a `STD_VER` of `21` is supposed to behave much more like `23` than like `20`. So in the past, the behavior should change as you move from `17` to `18`, not as you move from `19` to `20`.)


================
Comment at: libcxx/test/std/containers/sequences/vector.bool/enabled_hash.pass.cpp:16-17
 
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_CXX20_REMOVED_TYPE_TRAITS
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS
+
----------------
Re-remove.


================
Comment at: libcxx/test/std/utilities/function.objects/func.invoke/invoke.pass.cpp:101-104
     // Check that result_of_t matches Expect.
     typedef typename std::result_of<ClassFunc&&(Functor&&, NonCopyable&&)>::type
       ResultOfReturnType;
     static_assert((std::is_same<ResultOfReturnType, Expect>::value), "");
----------------
I'm ambivalent as to whether you should keep `_LIBCPP_ENABLE_CXX20_REMOVED_TYPE_TRAITS` at the top of the file, or simply put lines 101-104, 124-127, and 145-148 under `#if TEST_STD_VER <= 17` instead. I'm not requesting a change here, but maybe @ldionne will have a stronger opinion one way or the other.


================
Comment at: libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_type.pass.cpp:268-297
 // The example code specified in Note B for common_type
 namespace note_b_example {
 
 typedef bool (&PF1)();
 typedef short (*PF2)(long);
 
 struct S {
----------------
This code is from "Example 2," not "Note B," and it has literally nothing to do with `common_type`; it's an example demonstrating the use of `invoke_result_t` (presumably formerly `result_of`).
https://eel.is/c++draft/meta.trans.other#example-2

Since it has nothing to do with `common_type.pass.cpp`, I think these lines should simply be removed. (And then re-remove your added lines 13-15.)


================
Comment at: libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/result_of.deprecated.fail.cpp:9
 
-// UNSUPPORTED: c++03, c++11, c++14
+// REQUIRES: c++17
 
----------------
I think this should be
```
// UNSUPPORTED: c++03, c++11, c++14
// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_CXX20_REMOVED_TYPE_TRAITS
```
but defer to @ldionne since he just went through this with the `std::iterator` tests. (You could grep and check for what he did there. Maybe you already did.)

Same comment on meta.unary.prop/is_literal_type.deprecated.fail.cpp below.


================
Comment at: libcxx/test/support/poisoned_hash_helper.h:156-158
+  static_assert(can_hash<Hash const&, Key&>(), "");
+  static_assert(can_hash<Hash const&, Key const&>(), "");
+  static_assert(can_hash<Hash const&, Key&&>(), "");
----------------
We're now attaching the `&` on line 120, so please `s/Hash const&/Hash const/` here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102992