[libcxx-commits] [PATCH] D91004: [libc++] Implements concept destructible

Mon Nov 9 06:47:11 PST 2020

ldionne added a comment.

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?

================
Comment at: libcxx/test/std/concepts/concept.destructible/destructible.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
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.

================
Comment at: libcxx/test/std/concepts/concept.destructible/destructible.pass.cpp:56
+    static_assert( std::destructible<NoexceptDependant<int>>);
+    static_assert(!std::destructible<NoexceptDependant<double>>);
+}
----------------
Mordante wrote:
> miscco wrote:
> > Mordante wrote:
> > > cjdb wrote:
> > > > @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.
> > > 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.
> > 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
> 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 ?
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>)`.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91004/new/

https://reviews.llvm.org/D91004