Page MenuHomePhabricator

[libcxx][type_traits] remove `std::is_literal_type` and `std::result_of` for C++20

Authored by Wmbat on May 23 2021, 2:14 PM.



C++17 deprecated std::is_literal_type and std::result_of, C++20 removed them.

Implements parts of:

  • P0174R2 'Deprecating Vestigial Library Parts in C++17'.
  • P0619R4 'Reviewing Deprecated Facilities of C++17 for C++20'.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Wmbat added a comment.EditedMay 24 2021, 9:44 AM
  • Added an extra test for checking deprecation message
  • Removed extra lines and other minor problems
  • Replaced all instances of
#pragma clang diagnostic ignored "-Wdeprecated-declarations"
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"


Wmbat updated this revision to Diff 347440.May 24 2021, 10:12 AM

Fixes double commit problem

Wmbat updated this revision to Diff 347463.May 24 2021, 11:36 AM

Fix broken unit test

ldionne requested changes to this revision.May 25 2021, 12:42 PM
ldionne added inline comments.

What are we actually trying to test here? Can we instead check that std::is_trivial<Atomic> and do that under all standard modes?

46 ↗(On Diff #347463)

I don't think we're actually testing anything here, are we? I think we can just remove this without reducing coverage, since that's not how we should test that optional is constexpr-friendly. Also applies below.

This revision now requires changes to proceed.May 25 2021, 12:42 PM
Wmbat updated this revision to Diff 347817.May 25 2021, 4:14 PM

Added _LIBCPP_ENABLE_CXX20_REMOVED_IS_LITERAL_TYPE macro and fixed inconsistencies in some unit tests.

Quuxplusone requested changes to this revision.May 25 2021, 4:32 PM
Quuxplusone added inline comments.

Excellent noticing of documentation. :) However, on closer inspection of p0619r4, I think you should:

  • 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.


Same comments here (including the lack-of-need-to-alphabetize if you change the name of this macro)


Re-remove this.


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?]


Remove lines 21-23. We're only testing the deprecation warning here, so we only need the one line, line 20.


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:

20–21 ↗(On Diff #347271)

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.

14 ↗(On Diff #347817)

Remove this line.

This revision now requires changes to proceed.May 25 2021, 4:32 PM
Wmbat added inline comments.May 25 2021, 4:58 PM

result_of and result_of_t don't look deprecated or removed yet (in the type_traits header). Should I add them under the _LIBCPP_ENABLE_CXX20_REMOVED_TYPE_TRAITS macro documentation and not touch the code? I think might be missing something (likely) here.

Quuxplusone added inline comments.May 26 2021, 5:47 AM

Looks to me like they were removed in C++20:
and deprecated in-or-before C++17 (might need to dig to find exactly when).
And yes, I'm saying put the whole section D.13 under that same macro, _LIBCPP_ENABLE_CXX20_REMOVED_TYPE_TRAITS. And yes this will involve touching the code as well as the documentation. :)

Wmbat added inline comments.May 26 2021, 12:22 PM

So, I should deprecate & remove result_of in this commit as well? Looking at the source code of type_traits. result_of hasn't been removed or deprecated yet. I would have to rename the commit as the name wouldn't imply all that is being done in it.

Wmbat updated this revision to Diff 348665.May 29 2021, 4:59 PM

Now also deprecates and remove std::result_of

Wmbat retitled this revision from [libcxx][type_traits] deprecates `std::is_literal_type` and remove it for C++20 to [libcxx][type_traits] remove `std::is_literal_type` and `std::result_of` for C++20.May 29 2021, 5:01 PM
Wmbat edited the summary of this revision. (Show Details)
Quuxplusone accepted this revision as: Quuxplusone.May 29 2021, 5:24 PM

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


nit: lowercase "deprecated" and "removed" here and lines 236 and 305 below


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.)

16–17 ↗(On Diff #348665)



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.


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).

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.)


I think this should be

// UNSUPPORTED: c++03, c++11, c++14

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/ below.


We're now attaching the & on line 120, so please s/Hash const&/Hash const/ here.

ldionne added inline comments.May 31 2021, 7:59 AM

Let's keep it as-is. As a matter of policy, I also want to suggest that we don't provide those escape hatches anymore, so they'll be easier to remove if the tests are written as they are right now.


Agreed with Arthur - that way, we'll be running that test in all standard modes >= C++17, which is what we want (since we're enabling the removed type traits anyway). If we didn't have the _LIBCPP_ENABLE_CXX20_REMOVED_TYPE_TRAITS escape hatch, then I agree this should be // REQUIRES: c++17 instead.

Quuxplusone added inline comments.May 31 2021, 8:24 AM

@ldionne: we should take the policy thing to chat (Slack or Discord). FYI, I believe the "escape hatches" are strictly positively useful. I'd be perfectly amenable to saying "we don't really support users defining these in production." But I think they're great in terms of documenting our progress on omnibus papers like p0619, and also in terms of explaining otherwise-opaque _LIBCPP_STD_VER dependencies to the casual maintainer, and also (somewhat — maybe I'm ambivalent here) in terms of structuring the unit tests.

Wmbat updated this revision to Diff 348863.May 31 2021, 2:28 PM

Implementing suggestions from @Quuxplusone

LGTM again now.


Please move the ADDITIONAL_COMPILE_FLAGS line up to right below the UNSUPPORTED line.

Mordante accepted this revision as: Mordante.Jun 1 2021, 10:31 AM

Thanks for the updates, LGTM!

Wmbat updated this revision to Diff 349071.Jun 1 2021, 12:52 PM

Rebase on main + minor changes

Wmbat added inline comments.Jun 1 2021, 1:48 PM

For some reason, this static_assert fails under the GCC/C++20 build, but passes for all Clang builds. I have no idea why

ldionne requested changes to this revision.Jun 11 2021, 6:38 AM
ldionne added inline comments.

Okay, I dug a bit more into this and here's what I think we want:

  1. Remove this static assertion, it doesn't test anything anymore.
  2. Add libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/dtor.pass.cpp, and add the following to it:
template <class Tp>
struct CheckTriviallyDestructible {
    void operator()() const {
        typedef std::atomic<Tp> Atomic;
        static_assert(std::is_trivially_destructible<Atomic>::value, "");

TestEachFloatingPointType<CheckTriviallyDestructible>()(); // needs to be added to `atomic_helpers.h`
TestEachPointerType< CheckTriviallyDestructible >()(); // needs to be added to `atomic_helpers.h`
  1. Add libcxx/test/std/atomics/atomics.types.generic/standard_layout.compile.pass.cpp and add the following in it:
template <class Tp>
struct CheckStandardLayout {
    void operator()() const {
        typedef std::atomic<Tp> Atomic;
        static_assert(std::is_standard_layout<Atomic>::value, "");


In other words, make sure that std::atomic<bool|Integral|FloatingPoint|Pointer> is standard layout, and that std::atomic<Integral|FloatingPoint|Pointer> is trivially destructible. I think that's pretty much the extent of the guarantees provided by the standard.


Yeah, I would remove this test entirely. I don't think it's checking anything useful anymore.


Why not write this as:

typedef std::result_of<CallableStruct(int)>::type f; // expected-warning {{'result_of<CallableStruct (int)>' is deprecated}}

Unless there is a reason for the funky indentation?

This revision now requires changes to proceed.Jun 11 2021, 6:38 AM
cjdb added inline comments.Jun 11 2021, 8:40 AM

clang-format doesn't like that (I suspect the line is longer than 120 chars). I do this instead:

typedef std::result_of<CallableStruct(int)>::type f;
// expected-warning@-1{{'result_of<CallableStruct (int)>' is deprecated}}
Quuxplusone added inline comments.Jun 11 2021, 8:52 AM

Another way of writing this test is as a two-liner:

std::result_of<int(*())()> a; // expected-warning{{is deprecated}}
std::result_of_t<int(*())()> b; // expected-warning{{is deprecated}}

(and then don't even provide a main because this test is compile-only).

Wmbat updated this revision to Diff 354326.Jun 24 2021, 11:59 AM

Changes based on recommendations.

  • TestEachPointerType does not check for all pointer types yet. Open to suggestions for anything i may miss
  • Fixed weird formatting in

Continues to LbasicallyGTM. Please rebase on trunk and re-upload the diff, so that we can get a clean buildkite run; I see the latest run failed some tests that have been fixed in main for a while already.


Please move this up to right below line 9, as in meta.trans.other/

Wmbat updated this revision to Diff 354752.Jun 27 2021, 8:44 AM

Rebased on main

ldionne requested changes to this revision.Jun 28 2021, 9:21 AM

Almost LGTM, the only issue I have with this is the duplication of the atomic_helpers.h header, for which there's a better solution available.

1 ↗(On Diff #354752)

Why are we duplicating this file? Instead, let's move the original file to libcxx/test/support/atomic_helpers.h and include it as just #include "atomic_helpers.h" from everywhere.

This revision now requires changes to proceed.Jun 28 2021, 9:21 AM
Wmbat updated this revision to Diff 355082.Jun 28 2021, 6:28 PM

Modifications based on @ldionne's recommendations

Wmbat updated this revision to Diff 355086.Jun 28 2021, 6:53 PM

Fixed incorrect header to atomic_helpers.h in atomic_wait.pass.cpp

ldionne accepted this revision.Jun 29 2021, 7:37 AM



As a fly-by, can you fix this comment to say // ATOMIC_HELPERS_H (missing S). Don't re-upload a patch just for that.

This revision is now accepted and ready to land.Jun 29 2021, 7:37 AM