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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 4 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/test/std/concepts/concept.destructible/destructible.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
ldionne wrote:
> 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.
Nice to know, I wasn't aware of this feature.


================
Comment at: libcxx/test/std/concepts/concept.destructible/destructible.pass.cpp:56
+    static_assert( std::destructible<NoexceptDependant<int>>);
+    static_assert(!std::destructible<NoexceptDependant<double>>);
+}
----------------
ldionne wrote:
> 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>)`.
> 
I agree with both parts, in general not to depend on implementation details and that this is a special case.


================
Comment at: libcxx/test/std/concepts/concept.destructible/destructible.pass.cpp:41
+constexpr bool test() {
+    assert( std::destructible<Empty>);
+
----------------
miscco wrote:
> Mordante wrote:
> > miscco wrote:
> > > I believe these should all be static_asserts and we should not need to have a "runtime" check of the trait
> > I just verified with the type_traits and they only use `static_asserts`, so I agree we should do the same with concepts.
> 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?
Thanks for the suggestion.


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

https://reviews.llvm.org/D91004