This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Summary

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

Wmbat requested review of this revision.May 23 2021, 2:14 PM
Wmbat created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2021, 2:14 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Wmbat edited the summary of this revision. (Show Details)May 23 2021, 2:17 PM
cjdb added a comment.May 23 2021, 2:18 PM

Overall LGTM, thanks for working on this! Please also add a test to confirm that std::is_literal_type is deprecated in C++17.

libcxx/test/std/utilities/meta/meta.unary/meta.unary.prop/is_literal_type.removed.verify.cpp
8–9
16–17

Please delete these two lines.

LGTM % comments, but please fix the review comments and wait for "libc++" to accept.

libcxx/include/type_traits
3721

Please remove blank lines 3721 and 3731.

libcxx/test/std/utilities/any/any.class/not_literal_type.pass.cpp
15–16

// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS
(git grep -i deprecat libcxx/test/ finds this idiom quickly)

libcxx/test/std/utilities/meta/meta.unary/meta.unary.prop/is_literal_type.removed.verify.cpp
17–18

Oh— you probably need to do something here to deal with non-Clang compilers. @ldionne probably knows what to do.

Mordante requested changes to this revision.May 24 2021, 8:20 AM

Thanks for working on this. Please update this note https://libcxx.llvm.org/docs/Cxx2aStatus.html#note-p0619
I see some build issues, but I expect @cjdb's suggestions to fix them.

libcxx/test/std/utilities/meta/meta.unary/meta.unary.prop/is_literal_type.removed.verify.cpp
17–18

Since the test is a verify.cpp it should only be executed with the Clang compiler.

libcxx/test/std/utilities/optional/optional.object/optional.object.dtor/dtor.pass.cpp
50

Since we disable some tests in C++20 (and later) modes it would be nice to test some other properties. regarding copy constructible/assignable and move constructible/assignable.

This revision now requires changes to proceed.May 24 2021, 8:20 AM
Wmbat added inline comments.May 24 2021, 9:01 AM
libcxx/test/std/utilities/optional/optional.object/optional.object.dtor/dtor.pass.cpp
50

Are those (copy constructible/assignable and move constructible/assignable) really necessary to test in this file which is about dtor.pass.cpp. I have seen some tests for those properties elsewhere in the meta.unary.pop folder. Would it be possible to elaborate further on what I would have to do?

Mordante added inline comments.May 24 2021, 9:09 AM
libcxx/test/std/utilities/optional/optional.object/optional.object.dtor/dtor.pass.cpp
50

In that case these tests aren't needed. I just felt it would be nice to have tests instead of the disabled literal_type test.

50

Would it be possible to elaborate further on what I would have to do?

Not entirely sure what you mean. I marked the patch as 'needs work' due to the build failures. Others already offered suggestions how to fix these.

Wmbat added inline comments.May 24 2021, 9:19 AM
libcxx/test/std/utilities/optional/optional.object/optional.object.dtor/dtor.pass.cpp
50

Would it be possible to elaborate further on what I would have to do?

Oh, sorry for the misunderstanding. This statement was referring to adding tests for the other properties. I have been working on the build failures and all recommendation, I was just wondering about the extra tests

Wmbat updated this revision to Diff 347429.May 24 2021, 9:44 AM
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"

with

// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS
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 is_literal_type.deprecated.fail.cpp unit test

ldionne requested changes to this revision.May 25 2021, 12:42 PM
ldionne added inline comments.
libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/ctor.pass.cpp
40

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

libcxx/test/std/utilities/optional/optional.object/optional.object.dtor/dtor.pass.cpp
44–45

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.
libcxx/docs/UsingLibcxx.rst
266–267

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.

libcxx/include/__config
1375

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

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/ctor.pass.cpp
16

Re-remove this.

libcxx/test/std/utilities/any/any.class/not_literal_type.pass.cpp
23

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

libcxx/test/std/utilities/meta/meta.unary/meta.unary.prop/is_literal_type.deprecated.fail.cpp
21–23

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

libcxx/test/std/utilities/meta/meta.unary/meta.unary.prop/is_literal_type.pass.cpp
9

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
libcxx/test/std/utilities/meta/meta.unary/meta.unary.prop/is_literal_type.removed.verify.cpp
17–18

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.

libcxx/test/std/utilities/optional/optional.object/optional.object.dtor/dtor.pass.cpp
14

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
libcxx/docs/UsingLibcxx.rst
266–267

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.

libcxx/docs/UsingLibcxx.rst
266–267

Looks to me like they were removed in C++20:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0619r4.html#3.12
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
libcxx/docs/UsingLibcxx.rst
266–267

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)

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

libcxx/include/type_traits
169–170

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

3720–3721

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

libcxx/test/std/containers/sequences/vector.bool/enabled_hash.pass.cpp
16–17 ↗(On Diff #348665)

Re-remove.

libcxx/test/std/utilities/function.objects/func.invoke/invoke.pass.cpp
101–104 ↗(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.

libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_type.pass.cpp
268–297 ↗(On Diff #348665)

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

libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/result_of.deprecated.fail.cpp
9 ↗(On Diff #348665)

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.

libcxx/test/support/poisoned_hash_helper.h
156–158 ↗(On Diff #348665)

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
libcxx/test/std/utilities/function.objects/func.invoke/invoke.pass.cpp
101–104 ↗(On Diff #348665)

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.

libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/result_of.deprecated.fail.cpp
9 ↗(On Diff #348665)

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.

libcxx/test/std/utilities/function.objects/func.invoke/invoke.pass.cpp
101–104 ↗(On Diff #348665)

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

libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/result_of.deprecated.fail.cpp
15 ↗(On Diff #348863)

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
libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/ctor.pass.cpp
40

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.
libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/ctor.pass.cpp
40

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, "");
    }
};

TestEachIntegralType<CheckTriviallyDestructible>()();
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, "");
    }
};

CheckStandardLayout<bool>();
TestEachIntegralType<CheckStandardLayout>()();
TestEachFloatingPointType<CheckStandardLayout>()();
TestEachPointerType<CheckStandardLayout>()();

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.

libcxx/test/std/utilities/any/any.class/not_literal_type.pass.cpp
23

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

libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/result_of.deprecated.fail.cpp
25–26 ↗(On Diff #349071)

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
libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/result_of.deprecated.fail.cpp
25–26 ↗(On Diff #349071)

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}}
libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/result_of.deprecated.fail.cpp
25–26 ↗(On Diff #349071)

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 result_of.deprecated.fail.cpp

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.

libcxx/test/std/utilities/meta/meta.unary/meta.unary.prop/is_literal_type.deprecated.fail.cpp
15

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

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.

libcxx/test/std/atomics/atomic_helpers.h
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

Thanks!

libcxx/test/support/atomic_helpers.h
142 ↗(On Diff #355086)

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