Page MenuHomePhabricator

[libc++] Implements concept constructible_from
ClosedPublic

Authored by Mordante on Nov 23 2020, 11:37 AM.

Details

Summary

Implements parts of:

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

Depends on: D91004

Diff Detail

Event Timeline

Mordante created this revision.Nov 23 2020, 11:37 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 23 2020, 11:37 AM
Mordante requested review of this revision.Nov 23 2020, 11:37 AM
cjdb added a comment.Nov 23 2020, 12:11 PM

@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.

@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.

Thanks for the suggestion and I will keep that in mind for other tests. However since this concept is fully specified by the Standard http://eel.is/c++draft/concept.constructible. I prefer to take the approach as suggested in D91004

@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>).

cjdb added a comment.Nov 24 2020, 9:57 AM

@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.

Thanks for the suggestion and I will keep that in mind for other tests. However since this concept is fully specified by the Standard http://eel.is/c++draft/concept.constructible. I prefer to take the approach as suggested in D91004

@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>).

@ldionne's comment isn't at odds with @CaseyCarter's. If anything, it's complementing it: void, function types, arrays, and pointer-to-members are all different types, none of which are being tested right now.

Mordante updated this revision to Diff 318770.Jan 23 2021, 10:21 AM
Mordante edited the summary of this revision. (Show Details)
  • Rebased.
  • Ran clang-format.
  • Added more unit tests at @cjdb's suggestion.
cjdb accepted this revision.Jan 23 2021, 10:23 AM

LGTM, pending approval from @EricWF or @ldionne

Thank you for adding the extra tests!

cjdb added inline comments.Jan 23 2021, 10:24 AM
libcxx/test/std/concepts/concept.constructible/constructible_from.compile.pass.cpp
42–46

Post-LGTM nit: do we really need this one if it's no longer standard?

Mordante marked an inline comment as done.Jan 23 2021, 10:53 AM

Thanks for the review.

libcxx/test/std/concepts/concept.constructible/constructible_from.compile.pass.cpp
42–46

What I mentioned was that ~Throw() throw(int); is no longer supported. However just saw this version is no longer deprecated but has been removed in C++20 http://eel.is/c++draft/diff.cpp17.except.

I'll update the patch. I also used this test in D91004, which I just committed. I'll commit a patch to also remove it in that test.

Mordante updated this revision to Diff 318776.Jan 23 2021, 11:01 AM
Mordante marked an inline comment as done.

The in C++11 deprecated throw() has been removed in C++20. Remove the test using this feature.

ldionne accepted this revision.Thu, Jan 28, 8:45 AM
This revision is now accepted and ready to land.Thu, Jan 28, 8:45 AM
This revision was automatically updated to reflect the committed changes.