This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement P2273R3 (`constexpr` `unique_ptr`)
ClosedPublic

Authored by fsb4000 on Aug 6 2022, 1:14 AM.

Details

Reviewers
Mordante
Group Reviewers
Restricted Project
Commits
rG30dadaa2eb49: [libc++] Implement P2273R3 (`constexpr` `unique_ptr`)

Diff Detail

Event Timeline

fsb4000 created this revision.Aug 6 2022, 1:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2022, 1:14 AM
fsb4000 requested review of this revision.Aug 6 2022, 1:14 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 6 2022, 1:14 AM
fsb4000 updated this revision to Diff 450493.Aug 6 2022, 1:20 AM

add more _LIBCPP_CONSTEXPR_CXX23

fsb4000 updated this revision to Diff 450497.Aug 6 2022, 1:37 AM

skip failed checks in compile time.

and I will continue modifying other unique_ptr tests.

philnik added a subscriber: philnik.Aug 6 2022, 2:47 AM

Thanks for working on this! This obviously needs more testing. I've got a few nit-picks, otherwise the diff looks good for now.

libcxx/include/__memory/unique_ptr.h
46–47

I'd prefer it if you leave the operator() on the same line.

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.special/cmp_nullptr.pass.cpp
52–55
fsb4000 updated this revision to Diff 450515.Aug 6 2022, 5:07 AM

I added some constexpr tests.

But many tests are not constexpr friendly and I don't know how to fix them :(

Not full list:

unique.ptr.special/eq.pass.cpp
unique.ptr.special/rel.pass.cpp
unique.ptr.special/swap.pass.cpp

unique.ptr.dltr.dflt1/default.pass.cpp
unique.ptr.dltr.dflt1/convert_ctor.pass.cpp

unique.ptr.dltr.dflt/default.pass.cpp
unique.ptr.dltr.dflt/convert_ctor.pass.cpp

unique.ptr.class/pointer_type.pass.cpp only static_asserts

unique.ptr.class/unique.ptr.observers/op_subscript.runtime.pass.cpp

/unique.ptr.class/unique.ptr.modifiers/release.pass.cpp
/unique.ptr.class/unique.ptr.modifiers/reset.pass.cpp
/unique.ptr.class/unique.ptr.modifiers/reset.single.pass.cpp
/unique.ptr.class/unique.ptr.modifiers/swap.pass.cpp

/unique.ptr.class/unique.ptr.ctor/pointer.pass.cpp
/unique.ptr.class/unique.ptr.ctor/pointer_deleter.pass.cpp

They use static variables and sometimes global variables...

fsb4000 updated this revision to Diff 450519.Aug 6 2022, 5:12 AM
fsb4000 marked an inline comment as done.

fix one nit-pick

fsb4000 marked an inline comment as done.Aug 6 2022, 5:16 AM

It seems my clang-format 14 is too old or I use it wrong.

I use

$ llvm-project\libcxx\clang-format -i file_name.cpp

Do you use clang-format 15 or 16?

Thanks for working on this! Since the patch is marked as WIP I didn't validate everything, but in general it looks fine. However the formatting changes make it a bit hard to see all relevant changes.

libcxx/include/__memory/unique_ptr.h
31

Please also update the synopsis in memory to mention the addition of constexpr.

36

In general we only format the changes in the diff instead of the entire file. That makes reviewing a bit easier.
(Unlike MSVC STL we (IMO unfortunately) haven't applied clang-format to our entire code base.)

513

This isn't used in C++23.

553

You missed this one after rebasing

561

The three operators here are also not used in C++23

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.ctor/default.pass.cpp
105

We normally ignore the result of these tests except when we use it in a static_assert. Basically we use static_assert to force compile-time evaluation.

fsb4000 updated this revision to Diff 450629.Aug 7 2022, 8:10 AM

I updated the synopsis in memory to mention the addition of constexpr and rewrote my changes of unique_ptr.h without clang-format

fsb4000 updated this revision to Diff 450630.Aug 7 2022, 8:25 AM
fsb4000 marked 5 inline comments as done.

ignore the result of these tests except when we use it in a static_assert

fsb4000 retitled this revision from [WIP] [libc++] Implement P2273R3 (`constexpr` `unique_ptr`) to [libc++] Implement P2273R3 (`constexpr` `unique_ptr`).Aug 7 2022, 8:27 AM
fsb4000 marked an inline comment as done.

I renamed the revision but I'm still unsure if these tests are enough.

libcxx/include/__memory/unique_ptr.h
36

Yes, sorry :(
I updated unique_ptr.h without clang-format so it should be easier for review now.

fsb4000 edited the summary of this revision. (Show Details)Aug 7 2022, 8:28 AM
fsb4000 updated this revision to Diff 450633.Aug 7 2022, 9:01 AM

fix test

fsb4000 updated this revision to Diff 450736.Aug 8 2022, 2:06 AM

libcxx\test\std\utilities\smartptr\unique.ptr\unique.ptr.create\make_unique.sizezero.pass.cpp is not constexpr friendly.

SGTM, but some requests remain, once done I'll have a closer look; probably next weekend.

libcxx/include/memory
409

We normally add the constexpr to the wording too, that way the wording in the synopsis matches the Standard. (Except we don't remove entries.)

libcxx/test/support/test_comparisons.h
32 ↗(On Diff #450736)

Can you mention what changed in this file?

fsb4000 updated this revision to Diff 452215.Aug 12 2022, 10:19 AM

revert test_comparisons.h changes and add constexpr in comments.

fsb4000 updated this revision to Diff 452221.Aug 12 2022, 10:25 AM

one more comment change.

fsb4000 updated this revision to Diff 452236.Aug 12 2022, 10:57 AM
fsb4000 marked an inline comment as done.

add Microsoft STL test.

fsb4000 updated this revision to Diff 452336.Aug 12 2022, 5:37 PM

move => std::move

fsb4000 updated this revision to Diff 452384.Aug 12 2022, 10:27 PM

rerun tests

While checking I missed several tests, but it seems you mentioned that.

I added some constexpr tests.

But many tests are not constexpr friendly and I don't know how to fix them :(

Not full list:

/unique.ptr.class/unique.ptr.modifiers/reset.pass.cpp

They use static variables and sometimes global variables...

I have looked at a solution and for the reset test I've created a fix. This basically removes the parts that can't be tested in a constexpr way; in this case a static class member. So this means our coverage during constant evaluation is a bit lower, but we can still test the basics. (We've done this in other tests too.) Maybe some day when the constexpr restrictions will be reduced further we can then re-enable these tests.

I've tested my patch locally with all supported language standards, feel free to use this in your patch.
http://paste.debian.net/1250391/

(I want to validate this patch against the paper when all tests are done.)

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/constexpr.pass.cpp
9 ↗(On Diff #452384)

I would propose a different way to include this test, in a similar fashion we did with the charconv tests

  • make a subdirectory p2273r3.msvc
  • rename this file to test.compile.cpp and remove all libc++ specifics (except for the overwrite when it doesn't compile) like the EDG work-arounds
  • add a test.compile.pass.cpp that has the libc++ specific which includes test.compile.cpp, in a similar fashion as like test/std/utilities/charconv/charconv.msvc/test.pass.cpp

This means the diff between us and MSVC STL remains minimal. (I still want to look at using more MSVC STL tests in the future, but that's very low on my priority list.)

142 ↗(On Diff #452384)

I wonder whether that's needed, but I'll ask the MSVC STL team, we don't use it for compile-time only tests.

fsb4000 updated this revision to Diff 454166.Aug 19 2022, 11:38 PM

Rebase and fix conflicts.
Also rename constexpr.pass.cpp to constexpr.compile.pass.cpp and add comment "// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20"

fsb4000 updated this revision to Diff 454186.Aug 20 2022, 2:47 AM

add more tests and move the msvc test to unique.ptr.msvc folder.

fsb4000 updated this revision to Diff 454188.Aug 20 2022, 2:55 AM

add missing newline to the end of some files.

fsb4000 updated this revision to Diff 454197.Aug 20 2022, 4:08 AM

_LIBCPP_CONSTEXPR_CXX23 => _LIBCPP_CONSTEXPR_SINCE_CXX23

fsb4000 updated this revision to Diff 454206.Aug 20 2022, 6:39 AM

fix -Wunreachable-code warning

fsb4000 updated this revision to Diff 454866.Aug 23 2022, 9:03 AM

refactor tests a bit.

In general LGTM but several minor issue.
When you update the patch please do *not* rebase. That makes it a lot easier to just view the diff.

libcxx/docs/Status/Cxx2bPapers.csv
47

Can you add a note that make_unique_for_overwrite isn't done yet since P1020 hasn't been implemented yet.

libcxx/include/memory
437

Nice catch!

479

It seems

template<class U, class E>
  constexpr unique_ptr(unique_ptr<U, E>&& u) noexcept;

is missing. In general this section seems to already miss template<class U>.

488–491

Pre existing issue: here too seems to be a member to be missing.

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.ctor/nullptr.pass.cpp
13

Please update the synopsis. Also validate the other tests please.

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.modifiers/reset_self.pass.cpp
16

There's no assert added.

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/op_subscript.runtime.pass.cpp
21
26–33

Combined with setting the default value to 0 this seems easier to read.
Before C++23 the code will never be constant evaluated.

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.dltr/unique.ptr.dltr.dflt/convert_ctor.pass.cpp
50

Please add return 0;.

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.dltr/unique.ptr.dltr.dflt/default.pass.cpp
46

Please add return 0;.

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.dltr/unique.ptr.dltr.dflt1/convert_ctor.pass.cpp
45

Please add return 0;.

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.dltr/unique.ptr.dltr.dflt1/default.pass.cpp
48

Please add return 0;.

fsb4000 updated this revision to Diff 457003.Aug 31 2022, 10:11 AM
fsb4000 marked 12 inline comments as done.

code review from Mordante

fsb4000 updated this revision to Diff 457311.Sep 1 2022, 10:02 AM

fix code formatting

fsb4000 updated this revision to Diff 457482.Sep 1 2022, 7:49 PM

compatibility with C++03

fsb4000 updated this revision to Diff 457763.Sep 2 2022, 10:25 PM

rebase after forward_likepatch was commited.
Fix conflict at libcxx\docs\ReleaseNotes.rst

fsb4000 updated this revision to Diff 457771.Sep 3 2022, 1:19 AM

forward_like.msvc tests CRLF => LF

Mordante accepted this revision.Sep 3 2022, 4:43 AM

Thanks a lot, LGTM! One small remark remaining, but not really related to this patch itself.

libcxx/test/std/utilities/utility/forward_like/forward_like.msvc/test.pass.cpp
1 ↗(On Diff #457776)

This file is already fixed in main.

This revision is now accepted and ready to land.Sep 3 2022, 4:43 AM
This revision was automatically updated to reflect the committed changes.
fsb4000 marked an inline comment as done.

Thanks for the patch. I just stumbled on this and noticed that it was adding the MSVC test. In general, let's try to write our tests ourselves and keep them consistent with the libc++ testing style. This avoids falling into a trap where all implementations could have the same issue because they all use the same tests. This comment doesn't apply for significant things like std::to_chars that we literally copied the implementation from MSVC, however in this case it's pretty simple.