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

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

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


================
Comment at: libcxx/docs/UsingLibcxx.rst:266-267
 
+**_LIBCPP_ENABLE_CXX20_REMOVED_IS_LITERAL_TYPE**:
+  This macro is used to re-enable `is_literal_type` and `is_literal_type_v`.
 
----------------
Excellent noticing of documentation. :) However, on closer inspection of p0619r4, I think you should:
- rename to `_LIBCPP_ENABLE_CXX20_REMOVED_TYPE_TRAITS`
- include `result_of` and `result_of_t` under the same switch
- (and then you //won't// need to alphabetize, because `TY...`, unlike `IS...`, comes after `RA...`)

Then you can mark p0619 section D.13 as complete.


================
Comment at: libcxx/include/__config:1375
 #define _LIBCPP_ENABLE_CXX20_REMOVED_RAW_STORAGE_ITERATOR
+#define _LIBCPP_ENABLE_CXX20_REMOVED_IS_LITERAL_TYPE
 #endif // _LIBCPP_ENABLE_CXX20_REMOVED_FEATURES
----------------
Same comments here (including the lack-of-need-to-alphabetize if you change the name of this macro)


================
Comment at: libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/ctor.pass.cpp:16
 
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS
+
----------------
Re-remove this.


================
Comment at: libcxx/test/std/utilities/any/any.class/not_literal_type.pass.cpp:23
 int main(int, char**) {
     static_assert(!std::is_literal_type<std::any>::value, "");
 
----------------
I think @ldionne's comment on the `optional` test also applies here: This test is merely testing that `std::any` isn't constexpr-constructible, specifically in C++17? I don't think that's a useful test. I think this entire file should be deleted and never spoken of again. :)  [Any objections?]


================
Comment at: libcxx/test/std/utilities/meta/meta.unary/meta.unary.prop/is_literal_type.deprecated.fail.cpp:21-23
+  static_assert(
+      std::is_literal_type<const int>::value, // expected-warning {{'is_literal_type<const int>' is deprecated}}
+      "");
----------------
Remove lines 21-23. We're only testing the deprecation warning here, so we only need the one line, line 20.


================
Comment at: libcxx/test/std/utilities/meta/meta.unary/meta.unary.prop/is_literal_type.pass.cpp:9
 
+// REQUIRES: c++11, c++14, c++17
+
----------------
Remove this line. (This would be wrong anyway, since we never pass `c++11` and `c++14` and `c++17` all at the same time, right?)

Instead, add above line 15:
```
// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_CXX20_REMOVED_TYPE_TRAITS
```


================
Comment at: libcxx/test/std/utilities/meta/meta.unary/meta.unary.prop/is_literal_type.removed.verify.cpp:20-21
 
-  return 0;
-}
+using std::is_literal_type;   // expected-error {{no member named 'is_literal_type' in namespace 'std'}}
+using std::is_literal_type_v; // expected-error {{no member named 'is_literal_type_v' in namespace 'std'}}
----------------
Mordante wrote:
> Quuxplusone wrote:
> > Oh— you probably need to do something here to deal with non-Clang compilers. @ldionne probably knows what to do.
> Since the test is a `verify.cpp` it should only be executed with the Clang compiler.
I've changed my mind about the usefulness of this test: This test is //not// useful. There's a reason we don't have any similar tests for other removed facilities — we want all the tests in `libcxx/test/std/` to be applicable to ANY conforming standard library, including e.g. libstdc++. And AFAIK it's perfectly conforming for a standard library to provide the name `std::is_literal_type` even in C++20, as a conforming extension.

So, I recommend deleting this whole file.


================
Comment at: libcxx/test/std/utilities/optional/optional.object/optional.object.dtor/dtor.pass.cpp:14
 
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS
+
----------------
Remove this line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102992