Page MenuHomePhabricator

[libcxx][optional] adds missing constexpr operations
ClosedPublic

Authored by cjdb on May 8 2021, 6:43 PM.

Details

Summary

Makes the following operations constexpr:

  • std::swap(optional, optional)
  • optional(optional<U> const&)
  • optional(optional<U>&&)
  • ~optional()
  • operator=(nullopt_t)
  • operator=(U&&)
  • operator=(optional<U> const&)
  • operator=(optional<U>&&)
  • emplace(Args&&...)
  • emplace(initializer_list<U>, Args&&...)
  • swap(optional&)
  • reset()

P2231 has been accepted by plenary, with the committee recommending
implementers retroactively apply to C++20. It's necessary for us to
implement _`semiregular-box`_ and _`non-propagating-cache`_, both of
which are required for ranges (otherwise we'll need to reimplement
std::optional with these members constexprified).

Diff Detail

Event Timeline

cjdb requested review of this revision.May 8 2021, 6:43 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2021, 6:43 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb updated this revision to Diff 343882.May 8 2021, 7:14 PM

replaces placement new with std::construct_at in C++20

cjdb updated this revision to Diff 343883.May 8 2021, 7:58 PM
  • Gets C++17 mode working.
  • GCC 10 doesn't like this patch, but GCC 11 mostly does
cjdb updated this revision to Diff 343886.May 8 2021, 9:42 PM
  • disables GCC for constexpr tests (should this be temporary or permanent?)
  • removes tests using is_literal_type since that was deprecated in C++17 and is to be removed in C++20
cjdb updated this revision to Diff 343934.May 9 2021, 1:59 PM

updates C++2b status paper

This mostly looks good to me. A few comments, but mainly minor things.

The following two tests look like they still need to be updated:

  • libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/explicit_optional_U.pass.cpp
  • libcxx/test/std/utilities/optional/optional.object/optional.object.assign/optional_U.pass.cpp
libcxx/docs/Cxx2bStatusPaperStatus.csv
13 ↗(On Diff #343934)

Was this paper accepted yet?

libcxx/include/optional
105–106

This should be updated, too.

105–106

And these assignment operators.

326

Is this part of another paper? Should we add a test too?

libcxx/test/std/utilities/optional/optional.object/optional.object.assign/emplace_initializer_list.pass.cpp
72

This could just be constexpr, no?

86

Nit: checkY or testY (better, imho).

libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/move.fail.cpp
23

Want to update this comment?

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

We should still test this pre-c++20, I think. You could group them all together at the end, though.

libcxx/test/std/utilities/optional/optional.object/optional.object.swap/swap.pass.cpp
138

I think the tests here are sufficient. But if you wanted, you could use the same dtor count trick as you did above (using a pointer) and then factor the existing tests out into check_swap and get rid of W.

As I said, either way is OK. I think there's sufficient set coverage here.

cjdb added inline comments.Sun, May 16, 9:26 PM
libcxx/docs/Cxx2bStatusPaperStatus.csv
13 ↗(On Diff #343934)

Please read my commit message.

libcxx/include/optional
326

This is necessary for GCC 11 to like this patch. It the two should be equivalent; but this one is permissible in a constant expression.

libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/move.fail.cpp
23

This is still relevant for C++17?

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

🤷 std::is_literal_type was deprecated in C++17 with the rationale

The traits had unreliable or awkward interfaces. The is_literal_type trait provided no way to detect which subset of constructors and member functions of a type were declared constexpr.

I'm not sure our tests should have something unreliable?

tcanens added inline comments.Sun, May 16, 9:52 PM
libcxx/test/std/utilities/optional/optional.object/optional.object.dtor/dtor.pass.cpp
43

The problem with is_literal_type is that it provides an answer to a mostly useless question - knowing that a type has some constexpr constructor tells you pretty much nothing about what you can do with it. The trait itself is perfectly reliable.

ldionne requested changes to this revision.Thu, May 20, 10:49 AM
ldionne added inline comments.
libcxx/include/optional
72

Should be // constexpr in C++2b (in C++20 as an extension) or something along those lines, just to acknowledge that we're supporting that as an extension.

libcxx/test/std/utilities/optional/optional.object/optional.object.assign/emplace.pass.cpp
234

This should be:

#if TEST_STD_VER > 20 || (TEST_STD_VER >= 20 && defined(_LIBCPP_VER))

Basically, test it in C++2b, or in C++20 but only when testing libc++, since it is out own extension. Otherwise, a conforming implementation will fail the test suite in C++20 mode because they don't implement constexpr for std::optional. This applies everywhere.

Also, what's the deal with GCC? If it fails those tests, we should first report the issue to GCC and then use some XFAIL markup to disable it instead.

This revision now requires changes to proceed.Thu, May 20, 10:49 AM
cjdb added inline comments.Thu, May 20, 10:55 AM
libcxx/include/optional
72

Is it really an extension if LWG are recommending we backport it to C++20 as a "fix"?

libcxx/test/std/utilities/optional/optional.object/optional.object.assign/emplace.pass.cpp
234

Also, what's the deal with GCC? If it fails those tests, we should first report the issue to GCC and then use some XFAIL markup to disable it instead.

GCC 10 doesn't like our constexpr construct_at (GCC 11 is okay with it). I wasn't comfortable saying GCC couldn't test this file at all given it still works outside of constant expressions.

zoecarver added inline comments.Thu, May 20, 11:58 AM
libcxx/test/std/utilities/optional/optional.object/optional.object.assign/emplace.pass.cpp
234

I think you should mark the whole file as UNSUPPORTED: gcc-10. IMO it's better to test everything on one version of GCC than some things on no versions of GCC.

Do we still support GCC 10? When does that change go through? And when it does, can/should we remove all this UNSUPPORTED cruft?

cjdb added inline comments.Thu, May 20, 10:17 PM
libcxx/test/std/utilities/optional/optional.object/optional.object.assign/emplace.pass.cpp
234

Do we still support GCC 10?

We only support GCC 10.

And when it does, can/should we remove all this UNSUPPORTED cruft?

Why? It's not hurting anything to keep it there.

ldionne added inline comments.Mon, May 31, 2:56 PM
libcxx/include/optional
72

You're right - if the Standard says to backport it, then it's not an extension.

libcxx/test/std/utilities/optional/optional.object/optional.object.assign/emplace.pass.cpp
234

I would prefer that we use UNSUPPORTED: gcc-10. The reason is that UNSUPPORTED annotations disable the test for the given version of GCC which we know doesn't support <some feature>, but it doesn't disable it forever. When we start testing on GCC 11, the test will not be considered as UNSUPPORTED. This is a great way to make sure that our tests don't become useless in a given configuration - it will automatically start breaking in the new version, and we'll have to re-acknowledge the failure by adjusting the UNSUPPORTED markup.

On the contrary, guarding with TEST_COMPILER_GCC means that we'll most likely forget about this in the future, and those bits will never be tested on GCC.

libcxx/test/std/utilities/optional/optional.object/optional.object.assign/nullopt_t.pass.cpp
25

Why aren't we simply constexpr-ifying the tests like you did elsewhere?

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

I think Chris meant that our tests shouldn't be using a tool that we deprecated and removed. I agree with him, I don't think it's useful to check that optional<T> is a literal type (but my mind can easily be changed if you tell me why it would be useful to check this).

cjdb updated this revision to Diff 350766.Tue, Jun 8, 8:08 PM
cjdb marked 6 inline comments as done.
cjdb edited the summary of this revision. (Show Details)
  • updates tests
  • responds to @ldionne's feedback
ldionne requested changes to this revision.Wed, Jun 9, 8:55 AM

A few comments, and also CI should pass.

libcxx/docs/Cxx2bStatus.rst
42 ↗(On Diff #350766)

I don't think this note is useful anymore.

libcxx/test/std/utilities/optional/optional.object/optional.object.assign/emplace_initializer_list.pass.cpp
101

#if TEST_STD_VER > 17 ? (here and below)

This revision now requires changes to proceed.Wed, Jun 9, 8:55 AM
cjdb updated this revision to Diff 350914.Wed, Jun 9, 9:02 AM
cjdb marked an inline comment as done.
  • removes note
  • makes C++20 test only available in C++20
cjdb updated this revision to Diff 350927.Wed, Jun 9, 9:41 AM

guards around C++20 test

cjdb updated this revision to Diff 350933.Wed, Jun 9, 9:49 AM

final update to remove C++20 tests from C++17 mode

Quuxplusone added inline comments.Wed, Jun 9, 9:51 AM
libcxx/test/support/archetypes.h
56 ↗(On Diff #350927)

Style nit 1: static TEST_CONSTEXPR_CXX20 void
Correctness nit 2: None of these functions that touch static members like alive and constructed can actually be constexpr in C++20. Constexpr functions aren't allowed to modify (or even read) global data. Therefore, adding TEST_CONSTEXPR_CXX20 to all these functions is really just noise; you could remove it and simplify this patch.

ldionne accepted this revision.Wed, Jun 9, 9:54 AM

LGTM if CI passes, and the two nits applied.

libcxx/test/support/archetypes.h
56 ↗(On Diff #350927)

Agree with both.

I didn't care about the placement of static vs TEST_CONSTEXPR_CXX20, but I grepped and it is more consistent with existing style to use static TEST_CONSTEXPR_CXX20 than the reverse.

This revision is now accepted and ready to land.Wed, Jun 9, 9:54 AM
This revision was automatically updated to reflect the committed changes.