[libc++] Implements concept destructible

[Mordante]

Summary

Implements parts of:

• P0898R3 Standard Library Concepts
• P1754 Rename concepts to standard_case for C++20, while we still can

[miscco]

LGTM

If there is another earnest push for concepts / ranges we should get together with @cjdb, @ldionne to see whether we find a way to not step on our toes and duplicate work.

Also I note that the bottleneck has always been getting everything reviewed.

NOTE: I am not a maintainer so wait for one before you commit

libcxx/test/std/concepts/concept.destructible/destructible.pass.cpp

13 ↗

(On Diff #303646)

I believe it would improve readability of the test a lot if you would add = std::is_nothrow_constructible_v<T>

41 ↗

(On Diff #303646)

I believe these should all be static_asserts and we should not need to have a "runtime" check of the trait

48 ↗

(On Diff #303646)

Nitpick: Could you order this in front similar to the structs?

[Mordante]

In D91004#2381027, @miscco wrote:

LGTM

Thanks for the review!

If there is another earnest push for concepts / ranges we should get together with @cjdb, @ldionne to see whether we find a way to not step on our toes and duplicate work.

Well I'd love to get ranges available in libc++, so I'm quite motivated to work on them. Of course it would be great if others want to join the effort since it's not a small feature. @cjdb, @ldionne are you currently working on concepts/ranges or willing to join the effort?

libcxx/test/std/concepts/concept.destructible/destructible.pass.cpp

41 ↗

(On Diff #303646)

I just verified with the type_traits and they only use static_asserts, so I agree we should do the same with concepts.

[Mordante]

Addresses @miscco's review comments assert -> static_assert and some minor nits.

[cjdb]

@cjdb, @ldionne are you currently working on concepts/ranges or willing to join the effort?

Yeah, I've got a few open patches:

• D74292
• D77961
• D88131

I'd actually been hoping to merge my ranges library since ~March, but reviews are a major blocker and so I've put it on hold for now. Once @ldionne has the time to review, I'll resume contributing.

[cjdb]

libcxx/test/std/concepts/concept.destructible/destructible.pass.cpp

56 ↗

(On Diff #303719)

@CaseyCarter had this to say back in D74291.

There's tons of coverage here for class types and reference types. What about void, function types, pointers, arrays, pointers-to-members? (I don't know about clang/libc++, but the MSVC STL tests for type traits tend to _be_ the tests for compiler intrinsics used to implement those traits, so we consequently cover everything under the sun.)

I'd suggest we apply this here as well.

[Mordante]

In D91004#2381509, @cjdb wrote:

I'd actually been hoping to merge my ranges library since ~March, but reviews are a major blocker and so I've put it on hold for now. Once @ldionne has the time to review, I'll resume contributing.

I had a quick look at the library and it seems rather complete. Is it complete or are there still pieces missing?

libcxx/test/std/concepts/concept.destructible/destructible.pass.cpp

56 ↗

(On Diff #303719)

Normally I would agree with this comment. However the concept is just a small wrapper for is_nothrow_destructible_v<T> so I thought some sanity checks would suffice. The tests of is_nothrow_destructible already tests the type trait.

I've also created a not-yet-posted patch for constructible_from which takes the same approach.

For my WIP patch for default_initializable I'm busy adding a lot more tests since this concept is not a simple wrapper for a type trait. For that patch I feel the additional tests are required.

[miscco]

libcxx/test/std/concepts/concept.destructible/destructible.pass.cpp

56 ↗

(On Diff #303719)

I believe we should be as comprehensive as possible. There is no guarantee that in the future we would change the implementation or whatever. So each feature should get all the love it needs

41 ↗

(On Diff #303646)

Sorry that I forgot this the first time. We now have compile only tests that do not require any linking at all. This (and all other concepts tests) are prime examples of compile only tests and we should convert it to one. As far as I know it is only necessary to change the file ending to .compile.pass @ldionne?

[Mordante]

libcxx/test/std/concepts/concept.destructible/destructible.pass.cpp

56 ↗

(On Diff #303719)

Of course I can copy paste the tests from is_nothrow_destructible, but then there's no guarantee they'll remain in sync. What's your opinion @ldionne ?

[ldionne]

I'd like to address the issue of reviews, because it has been on my mind for some time.

I'm fully aware that reviews are a huge bottleneck and a source of frustration. I actually think that's the #1 problem that libc++ is facing these days, and has been for a while. I try to do my best to review upstream contributions, however the truth is that with me as the only full-time maintainer nowadays, it would be unrealistic to think that everything can be reviewed, let alone reviewed in a timely fashion. My job includes working on specific libc++ related projects (usually driven by internal priorities), and reviewing upstream contributions or even implementing new C++ features is unfortunately just a part of that. I'm trying not to rant -- I just want to explain how my time is spent on libc++ and why not 100% of it is devoted to implementing the Standard or reviewing patches.

One of the problems with contributing to libc++ is that there has been a very very high bar to doing so, and that's why maintainers have had to be involved in every change. There's a lot of configuration options, ways to build, guarantees that we provide to vendors and users, and specific knowledge around the libc++ test suite, infrastructure and conventions. I've been working really hard on lowering these barriers because I personally see them as the root cause of the problem:

1. I added pre-commit CI that runs all "supported" configurations when you submit a review, so that people don't have to know where to look on BuildBot or how to reproduce CI issues locally. Previously, about a third of the commits would break the post-commit CI bots and one of the maintainers would handle it. That doesn't scale.
2. I added the libc++ review group to remove the impression that committing to libc++ is gated on a specific individual. I wanted to open up the review process such that other folks could join and eventually be trusted enough to LGTM patches.
3. I've been progressively cleaning up support for old compilers, untested configurations and other similar simplifications. These accounted (and still do) for a lot of the added complexity when writing a patch, and they were the primary reasons why I had to get involved in contributions when they broke something. We need to push on getting rid of these sources of churn.
4. I've been trying to dumb down some recurring processes like re-generating ABI lists. Eric had already automated adding feature test macros with a python script, which is a godsend.

So, if you'd like to pursue a major effort like concepts or ranges, that's absolutely awesome. Unfortunately, being gated on my in-depth review is just not going to work if we want to make progress at a reasonable pace. What I would suggest is to setup some kind of working group with interested individuals (it looks like we already have some subscribed to this review). You can then iterate on patches between yourselves without involving the maintainers. Of course, if you have specific questions, ping me and I'll help out. I'll also add you to our Slack channel so we can communicate easily with minimal overhead.

Then, once CI is passing and everything looks alright, let me know and I'll take a brief look at whether things look okay from the libc++ side of things. I won't review the code itself in depth because I don't have the bandwidth, but I trust you folks with that. I might catch a few deviations from libc++isms, in which case I'll let you know and we can fix those. As we go, we can also create some sort of CONTRIBUTING.md document with a checklist of things to think about. I'm sure that would go a long way.

I really want to create a sane and efficient contribution culture around libc++, and I think it's necessary for the project to be successful. How does that sound?

libcxx/test/std/concepts/concept.destructible/destructible.pass.cpp

1 ↗

(On Diff #303719)

If this is a compile-time only test, you can use the .compile.pass.cpp extension instead. This will only compile the test, not run it, which can yield significant speed ups when running the test suite on slow environments like some embedded targets.

You don't even need a main() in that case.

56 ↗

(On Diff #303719)

In general, when writing tests for libc++, one should not assume knowledge of implementation-specific details for the following reasons:

1. The implementation may change in the future, and the tests should still provide us with sufficient confidence in that event.
2. Other standard libraries use our tests, and we can't assume they are implemented the same way.

*However*, this is not implementation-specific in this case, because the Standard (http://eel.is/c++draft/concept.destructible#lib:destructible) specifies that destructible is defined exactly as is_­nothrow_­destructible_­v<T>. It doesn't say "equivalent to is_­nothrow_­destructible_­v<T>" or something like that. I think the best way of testing that would be to throw a bunch of different types and say static_assert(std::destructible<T> == std::is_­nothrow_­destructible_­v<T>).

[Mordante]

In D91004#2382755, @ldionne wrote:

I'd like to address the issue of reviews, because it has been on my mind for some time.

<snip>

I really want to create a sane and efficient contribution culture around libc++, and I think it's necessary for the project to be successful. How does that sound?

Thanks for the verbose explanation! I like your proposal.
I think it would be a good idea to send this information to the libcxx-dev mailing. (If you already did then please disregard this request.)

[Mordante]

libcxx/test/std/concepts/concept.destructible/destructible.pass.cpp

1 ↗

(On Diff #303719)

Nice to know, I wasn't aware of this feature.

56 ↗

(On Diff #303719)

I agree with both parts, in general not to depend on implementation details and that this is a special case.

41 ↗

(On Diff #303646)

Thanks for the suggestion.

[Mordante]

Addresses the review comments:

• @miscco and @ldionne their suggestion to make the unit test a compile-time test
• @ldionne's suggestion to explicitly test the compatibility with is_nothrow_destructible_v

[miscco]

One last ask for @ldionne, currently MSVC`s internal test runner seems to require int main() even in compile only tests, as it not yet understands compileonly tests. For the sake ofcompatibility could we for now include int main() {} to enable cross vendor compatibility.

I would be happy to clean that up once it MSVC fixes their side

[Mordante]

In D91004#2383512, @miscco wrote:

One last ask for @ldionne, currently MSVC`s internal test runner seems to require int main() even in compile only tests, as it not yet understands compileonly tests. For the sake ofcompatibility could we for now include int main() {} to enable cross vendor compatibility.

I would be happy to clean that up once it MSVC fixes their side

I can add the following to the test if that help the MSVC team:

// Required for MSVC internal test runner compatibility.
int main(int, char**) {
    return 0;
}

[ldionne]

In D91004#2383512, @miscco wrote:

One last ask for @ldionne, currently MSVC`s internal test runner seems to require int main() even in compile only tests, as it not yet understands compileonly tests. For the sake ofcompatibility could we for now include int main() {} to enable cross vendor compatibility.

Yeah, I think it's OK to keep main() in that case. It certainly won't hurt.

[Mordante]

Added a main() to the compile time test as requested by @miscco .

[Mordante]

Change the unit tests to compile time test, based on the review comment in D91004.

[Mordante]

Undo the last revision since I accidentally attached a patch to the wrong review.

[Mordante]

Rebase to trigger CI.

[Mordante]

Rebase to trigger CI build.

[Mordante]

@ldionne the patch passes the CI build and I addressed all previous review comments. When you have time can you have a look?

[Mordante]

Implement all new header tests as suggested by @curdeius in D92214.

[Mordante]

Rebased.

[ldionne]

LGTM. Feel free to commit after fixing the minor comments.

libcxx/test/std/concepts/concept.destructible/destructible.compile.pass.cpp

21

We generally put a space before the opening brace of the struct. If only we had Clang-format :-).

51

We could also add tests for a few fundamental types like int.

[Mordante]

Thanks for the review. I'll address your comments and run a CI build before landing this revision.

[Mordante]

• Addresses review comments.
  • Add some fundamental types to the unit test
  • Ran clang-format on the patch
• Rebased to test against a recent main before landing the patch

libcxx/include/concepts

 // [concept.same]
 
 template<class _Tp, class _Up>
 concept __same_as_impl = _VSTD::_IsSame<_Tp, _Up>::value;
 
 template<class _Tp, class _Up>
 concept same_as = __same_as_impl<_Tp, _Up> && __same_as_impl<_Up, _Tp>;
 
+// [concept.destructible]
+
+template<class _Tp>
+concept destructible = _VSTD::is_nothrow_destructible_v<_Tp>;
+
 #endif //_LIBCPP_STD_VER > 17 && defined(__cpp_concepts) && __cpp_concepts >= 201811L
 
 _LIBCPP_END_NAMESPACE_STD
 
 _LIBCPP_POP_MACROS
 
 #endif // _LIBCPP_CONCEPTS

libcxx/test/std/concepts/concept.destructible/destructible.compile.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: libcpp-no-concepts
+
+// template<class T>
+// concept destructible = is_nothrow_destructible_v<T>;
+
+#include <concepts>
+#include <type_traits>
+
+struct Empty{};
+
+struct Defaulted{ ~Defaulted() = default; };
+struct Deleted{ ~Deleted() = delete; };
+
+struct Noexcept{ ~Noexcept() noexcept; };
+struct NoexceptTrue{ ~NoexceptTrue() noexcept(true); };
+struct NoexceptFalse{ ~NoexceptFalse() noexcept(false); };
+
+// Since C++17 dynamic exception specifications are no longer
+// part of the standard.
+struct Throw{ ~Throw() throw(); };
+
+struct Protected { protected: ~Protected() = default; };
+struct Private { private: ~Private() = default; };
+
+template<class T>
+struct NoexceptDependant {
+    ~NoexceptDependant() noexcept(std::is_same_v<T, int>);
+};
+
+template<class T>
+void test() {
+    static_assert(std::destructible<T> == std::is_nothrow_destructible_v<T>);
+}
+
+void test() {
+    test<Empty>();
+
+    test<Defaulted>();
+    test<Deleted>();
+
+    test<Noexcept>();
+    test<NoexceptTrue>();
+    test<NoexceptFalse>();
+
+    test<Throw>();
+
+    test<Protected>();
+    test<Private>();
+
+    test<NoexceptDependant<int>>();
+    test<NoexceptDependant<double>>();
+}
+
+// Required for MSVC internal test runner compatibility.
+int main(int, char**) {
+    return 0;
+}

libcxx/www/cxx2a_status.html

-->
   <tr><td><a href="https://wg21.link/P0722R3">P0722R3</a></td><td>CWG</td><td>Efficient sized delete for variable sized classes</td><td>Rapperswil</td><td>Complete</td><td>9.0</td></tr>
   <tr><td><a href="https://wg21.link/P0758R1">P0758R1</a></td><td>LWG</td><td>Implicit conversion traits and utility functions</td><td>Rapperswil</td><td>Complete</td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0759R1">P0759R1</a></td><td>LWG</td><td>fpos Requirements</td><td>Rapperswil</td><td>Complete</td><td>11.0</td></tr>
   <tr><td><a href="https://wg21.link/P0769R2">P0769R2</a></td><td>LWG</td><td>Add shift to &lt;algorithm&gt;</td><td>Rapperswil</td><td></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0788R3">P0788R3</a></td><td>LWG</td><td>Standard Library Specification in a Concepts and Contracts World</td><td>Rapperswil</td><td></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0879R0">P0879R0</a></td><td>LWG</td><td>Constexpr for swap and swap related functions Also resolves LWG issue 2800.</td><td>Rapperswil</td><td></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0887R1">P0887R1</a></td><td>LWG</td><td>The identity metafunction</td><td>Rapperswil</td><td>Complete</td><td>8.0</td></tr>
   <tr><td><a href="https://wg21.link/P0892R2">P0892R2</a></td><td>CWG</td><td>explicit(bool)</td><td>Rapperswil</td><td></td><td></td></tr>
-  <tr><td><a href="https://wg21.link/P0898R3">P0898R3</a></td><td>LWG</td><td>Standard Library Concepts</td><td>Rapperswil</td><td></td><td></td></tr>
+  <tr><td><a href="https://wg21.link/P0898R3">P0898R3</a></td><td>LWG</td><td>Standard Library Concepts</td><td>Rapperswil</td><td><i>In progress</i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0935R0">P0935R0</a></td><td>LWG</td><td>Eradicating unnecessarily explicit default constructors from the standard library</td><td>Rapperswil</td><td></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0941R2">P0941R2</a></td><td>CWG</td><td>Integrating feature-test macros into the C++ WD</td><td>Rapperswil</td><td><i>In progress</i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P1023R0">P1023R0</a></td><td>LWG</td><td>constexpr comparison operators for std::array</td><td>Rapperswil</td><td>Complete</td><td>8.0</td></tr>
   <tr><td><a href="https://wg21.link/P1025R1">P1025R1</a></td><td>CWG</td><td>Update The Reference To The Unicode Standard</td><td>Rapperswil</td><td></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P1120R0">P1120R0</a></td><td>CWG</td><td>Consistency improvements for &lt;=&gt; and other comparison operators</td><td>Rapperswil</td><td></td><td></td></tr>
 
    <tr><td></td><td></td><td></td><td></td><td></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0318R1">P0318R1</a></td><td>LWG</td><td>unwrap_ref_decay and unwrap_reference</td><td>San Diego</td><td>Complete</td><td>8.0</td></tr>
-->
   <tr><td><a href="https://wg21.link/P1614">P1614</a></td><td>LWG</td><td>The Mothership has Landed</td><td>Cologne</td><td><i>In progress</i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P1638">P1638</a></td><td>LWG</td><td>basic_istream_view::iterator should not be copyable</td><td>Cologne</td><td></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P1643">P1643</a></td><td>LWG</td><td>Add wait/notify to atomic_ref</td><td>Cologne</td><td></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P1644">P1644</a></td><td>LWG</td><td>Add wait/notify to atomic<shared_ptr></td><td>Cologne</td><td></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P1650">P1650</a></td><td>LWG</td><td>Output std::chrono::days with 'd' suffix</td><td>Cologne</td><td></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P1651">P1651</a></td><td>LWG</td><td>bind_front should not unwrap reference_wrapper</td><td>Cologne</td><td></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P1652">P1652</a></td><td>LWG</td><td>Printf corner cases in std::format</td><td>Cologne</td><td></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P1661">P1661</a></td><td>LWG</td><td>Remove dedicated precalculated hash lookup interface</td><td>Cologne</td><td><i>Nothing to do</i></td><td></td></tr>
-  <tr><td><a href="https://wg21.link/P1754">P1754</a></td><td>LWG</td><td>Rename concepts to standard_case for C++20, while we still can</td><td>Cologne</td><td></td><td></td></tr>
+  <tr><td><a href="https://wg21.link/P1754">P1754</a></td><td>LWG</td><td>Rename concepts to standard_case for C++20, while we still can</td><td>Cologne</td><td><i>In progress</i></td><td></td></tr>
 
    <tr><td></td><td></td><td></td><td></td><td></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0883">P0883</a></td><td>LWG</td><td>Fixing Atomic Initialization </td><td>Belfast</td><td><i> </i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P1391">P1391</a></td><td>LWG</td><td>Range constructor for std::string_view </td><td>Belfast</td><td><i> </i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P1394">P1394</a></td><td>LWG</td><td>Range constructor for std::span </td><td>Belfast</td><td><i> </i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P1456">P1456</a></td><td>LWG</td><td>Move-only views </td><td>Belfast</td><td><i> </i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P1622">P1622</a></td><td>LWG</td><td>Mandating the Standard Library: Clause 32 - Thread support library </td><td>Belfast</td><td><i> </i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P1645">P1645</a></td><td>LWG</td><td>constexpr for numeric algorithms </td><td>Belfast</td><td><i> </i></td><td></td></tr>