This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] adds concept std::derived_from
ClosedPublic

Authored by cjdb on Feb 8 2020, 9:16 PM.

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

cjdb created this revision.Feb 8 2020, 9:16 PM
miscco added inline comments.Feb 8 2020, 11:56 PM
libcxx/include/concepts
161–164

As before, this needs _Uglification.

Also note that is_base_of_v directly goes back to __is_base_of and you have the chain

__is_base_of -> i_base_of -> is_base_of_v

(See https://github.com/miscco/llvm-project/blob/bc29069dc401572ba62f7dd692a3474c1ead76c9/libcxx/include/type_traits#L1417-L1425)

So throughput would be better it you would again specialize for __clang__ and use __Is_base_of directly

CaseyCarter added inline comments.Feb 19 2020, 7:49 AM
libcxx/include/concepts
161–164

__is_base_of(Base, Derived) is available in all versions of clang/GCC/MSVC that support concepts (https://godbolt.org/z/xfLGbF).

__is_convertible_to(From, To) is available in clang/MSVC versions that support concepts (https://godbolt.org/z/jQEEr5). clang also supports the name __is_convertible.

cjdb updated this revision to Diff 256176.Apr 8 2020, 9:03 PM
cjdb marked 2 inline comments as done.

Updates tests to check for fundamental(ish) types, cv-qualifiers, references, pointers, void, functions, pointer-to-functions, reference-to-functions, pointer-to-member(-functions), and arrays.

cjdb added inline comments.Apr 8 2020, 10:37 PM
libcxx/test/std/concepts/lang/derived_from.pass.cpp
43 ↗(On Diff #256176)

I'm not sure if the rvalue-reference checks are necessary, or if static_assert(!std::derived_from<From*, To>) covers this.
CC @CaseyCarter

cjdb marked 2 inline comments as done.Apr 9 2020, 9:20 AM
cjdb added inline comments.
libcxx/test/std/concepts/lang/derived_from.pass.cpp
43 ↗(On Diff #256176)

Yes, they are, and that's the point of this check.

One correctness comment and some minor nits

libcxx/include/concepts
161–164

If I understand @CaseyCarter correctly then __is_convertible_to is not available with gcc so I guess we should use is_convertible_v

libcxx/test/std/concepts/lang/derived_from.pass.cpp
22 ↗(On Diff #256176)

I would suggest a better name maybe Derived_private and Derived_public instead of Derived1/Base2 same for protected

152 ↗(On Diff #256176)

That newline is not consistent with the other sections. That said i personally prefer a newline here.

cjdb updated this revision to Diff 293607.Sep 22 2020, 5:55 PM
cjdb marked 2 inline comments as done.
cjdb added a reviewer: ldionne.
cjdb removed a subscriber: ldionne.
cjdb added inline comments.Sep 22 2020, 5:55 PM
libcxx/test/std/concepts/lang/derived_from.pass.cpp
22 ↗(On Diff #256176)

Done.

152 ↗(On Diff #256176)

Newline is because of the comment.

cjdb added a comment.Sep 22 2020, 5:59 PM

This patch should be ready to check in, per @ldionne's approval.

ldionne added a reviewer: Restricted Project.Nov 2 2020, 2:23 PM
ldionne set the repository for this revision to rG LLVM Github Monorepo.Jan 28 2021, 8:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2021, 8:36 AM
ldionne accepted this revision.Jan 28 2021, 8:41 AM

I added the LLVM Monorepo to the revision - this will allow CI to trigger if you update the patch. Can you please rebase and re-upload? This LGTM if CI passes, but I think you might need to use is_convertible_v to fix the build on GCC. CI will tell.

This revision is now accepted and ready to land.Jan 28 2021, 8:41 AM
cjdb updated this revision to Diff 321300.Feb 3 2021, 7:43 PM

I added the LLVM Monorepo to the revision - this will allow CI to trigger if you update the patch. Can you please rebase and re-upload? This LGTM if CI passes, but I think you might need to use is_convertible_v to fix the build on GCC. CI will tell.

Done, but I don't see any CI panel showing up. Did I do something wrong?

cjdb updated this revision to Diff 321536.Feb 4 2021, 12:04 PM
cjdb set the repository for this revision to rG LLVM Github Monorepo.

Trying to activate CI

cjdb updated this revision to Diff 321635.Feb 4 2021, 7:35 PM

Selects __is_convertible_to or std::is_convertible_v based on compiler.

cjdb updated this revision to Diff 321637.Feb 4 2021, 7:40 PM

Helps if the right patch is uploaded.

cjdb updated this revision to Diff 321972.Feb 6 2021, 7:24 PM

CI seemed to fail, retrying

cjdb updated this revision to Diff 322009.Feb 7 2021, 5:07 PM
cjdb retitled this revision from [libcxx] adds [concept.derived] to [libcxx] adds concept std::derived_from.
cjdb edited the summary of this revision. (Show Details)

updates commit message

ldionne requested changes to this revision.Feb 9 2021, 3:30 PM
ldionne added inline comments.
libcxx/include/concepts
168

I thought I had left a comment here, but I would prefer that we de-duplicate this implementation and unconditionally use is_convertible_v (as mentioned on other patches).

This revision now requires changes to proceed.Feb 9 2021, 3:30 PM
cjdb updated this revision to Diff 322527.Feb 9 2021, 3:58 PM
cjdb added a subscriber: zoecarver.

applies @zoecarver's fix and @ldionne's fix

cjdb marked 2 inline comments as done.Feb 9 2021, 4:00 PM
ldionne accepted this revision.Feb 10 2021, 8:53 AM
This revision is now accepted and ready to land.Feb 10 2021, 8:53 AM
This revision was automatically updated to reflect the committed changes.