Details
- Reviewers
Mordante - Group Reviewers
Restricted Project - Commits
- rG30dadaa2eb49: [libc++] Implement P2273R3 (`constexpr` `unique_ptr`)
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
skip failed checks in compile time.
and I will continue modifying other unique_ptr tests.
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 | ||
---|---|---|
48–49 | 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 | ||
53–56 |
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...
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. | |
567 | This isn't used in C++23. | |
607 | You missed this one after rebasing | |
617 | 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–108 | 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. |
I updated the synopsis in memory to mention the addition of constexpr and rewrote my changes of unique_ptr.h without clang-format
I renamed the revision but I'm still unsure if these tests are enough.
libcxx/include/__memory/unique_ptr.h | ||
---|---|---|
36 | Yes, sorry :( |
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 | Can you mention what changed in this file? |
While checking I missed several tests, but it seems you mentioned that.
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
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. |
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"
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! | |
478 | 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>. | |
485–486 | 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 ↗ | (On Diff #454866) | |
26–33 ↗ | (On Diff #454866) | Combined with setting the default value to 0 this seems easier to read. |
libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.dltr/unique.ptr.dltr.dflt/convert_ctor.pass.cpp | ||
42 ↗ | (On Diff #454866) | Please add return 0;. |
libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.dltr/unique.ptr.dltr.dflt/default.pass.cpp | ||
37 ↗ | (On Diff #454866) | Please add return 0;. |
libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.dltr/unique.ptr.dltr.dflt1/convert_ctor.pass.cpp | ||
36 | Please add return 0;. | |
libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.dltr/unique.ptr.dltr.dflt1/default.pass.cpp | ||
39 ↗ | (On Diff #454866) | Please add return 0;. |
rebase after forward_likepatch was commited.
Fix conflict at libcxx\docs\ReleaseNotes.rst
rebase because https://github.com/llvm/llvm-project/commit/ff06c2ded367d71eee6e879b4c9934af35d7a7e8 already fixed one file.
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. |
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.
Can you add a note that make_unique_for_overwrite isn't done yet since P1020 hasn't been implemented yet.