Page MenuHomePhabricator

[libc++] Implements concept destructible
ClosedPublic

Authored by Mordante on Nov 7 2020, 8:26 AM.

Details

Summary

Implements parts of:

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

Diff Detail

Event Timeline

Mordante created this revision.Nov 7 2020, 8:26 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 7 2020, 8:26 AM
Mordante requested review of this revision.Nov 7 2020, 8:26 AM
miscco accepted this revision.Nov 7 2020, 1:08 PM
miscco added a subscriber: cjdb.

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 marked 3 inline comments as done.Nov 8 2020, 6:43 AM

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 updated this revision to Diff 303719.Nov 8 2020, 6:52 AM
Mordante marked an inline comment as done.

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

cjdb added a comment.Nov 8 2020, 8:48 AM

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

Yeah, I've got a few open patches:

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 added inline comments.Nov 8 2020, 8:52 AM
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.

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 added inline comments.Nov 8 2020, 11:24 AM
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 marked an inline comment as done.Nov 8 2020, 11:54 AM
Mordante added inline comments.
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 ?

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 marked an inline comment as done.Nov 9 2020, 8:11 AM

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 marked 4 inline comments as done.Nov 9 2020, 9:54 AM
Mordante added inline comments.
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 updated this revision to Diff 303913.Nov 9 2020, 9:59 AM
Mordante marked 3 inline comments as done.

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

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

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;
}

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 updated this revision to Diff 304235.Nov 10 2020, 9:31 AM

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

Mordante updated this revision to Diff 304240.Nov 10 2020, 9:46 AM

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

Mordante updated this revision to Diff 304253.Nov 10 2020, 10:33 AM

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

Mordante updated this revision to Diff 307377.Nov 24 2020, 8:53 AM
Mordante edited the summary of this revision. (Show Details)

Rebase to trigger CI.

Mordante updated this revision to Diff 307385.Nov 24 2020, 9:25 AM

Rebase to trigger CI build.

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

Mordante updated this revision to Diff 309288.Dec 3 2020, 9:39 AM
Mordante added a subscriber: curdeius.

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

ldionne accepted this revision.Jan 19 2021, 8:31 AM

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.

This revision is now accepted and ready to land.Jan 19 2021, 8:31 AM

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

Mordante updated this revision to Diff 318748.Jan 23 2021, 5:18 AM
  • 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
This revision was automatically updated to reflect the committed changes.