Page MenuHomePhabricator

[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

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"

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
33–34

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
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.
libcxx/docs/UsingLibcxx.rst
269–270

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
1362

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
22–24

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

libcxx/test/std/utilities/optional/optional.object/optional.object.dtor/dtor.pass.cpp
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
libcxx/docs/UsingLibcxx.rst
269–270

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
libcxx/docs/UsingLibcxx.rst
269–270

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
269–270

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.

libcxx/include/type_traits
168–170

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

3680–3681

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

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
263–264

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–10

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
157–159

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

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–10

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

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

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
33–34

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
33–34

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
22–23

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
22–23

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
libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/result_of.deprecated.fail.cpp
22–23

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
16

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
143

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