This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][type_traits] Implement C++20 common_ref
AbandonedPublic

Authored by miscco on Feb 10 2020, 11:22 AM.

Details

Reviewers
mclow.lists
EricWF
CaseyCarter
cjdb
ldionne
Group Reviewers
Restricted Project
Summary

This implements the new type trait common_ref that is needed for library concepts support

Event Timeline

miscco created this revision.Feb 10 2020, 11:22 AM
cjdb added inline comments.Feb 10 2020, 7:58 PM
libcxx/include/type_traits
2306–2307

Perhaps we should hold off on merging this so we can use std::convertible_to instead.

miscco added a comment.EditedFeb 10 2020, 11:53 PM

@cjdb I have to disagree. That would create a ciruclar dependency between concepts and type_traits, which is in my opinion not worth it.

Also note that std::convertible_to essentially falls back to is_convertible_v

miscco marked an inline comment as done.Feb 11 2020, 12:06 AM
miscco added inline comments.
libcxx/include/type_traits
2292–2318

I think I should add the respective wording here so that one understands what it is actually testing.

miscco updated this revision to Diff 243786.Feb 11 2020, 3:19 AM

Simplified code and improved comments

miscco added inline comments.Feb 11 2020, 3:24 AM
libcxx/include/type_traits
2306–2307

I would add that std::convertible_to has slightly different semantics too as it has some addtional requires clauses

cjdb added inline comments.Feb 12 2020, 11:22 AM
libcxx/include/type_traits
2306–2307

Fair. I'd at least encourage the use of a _requires-clause_, since it's demonstrably faster to compile than SFINAE and is being commit for the first time to a concepts-aware compiler.

miscco marked an inline comment as done.Feb 12 2020, 12:06 PM
miscco added inline comments.
libcxx/include/type_traits
2306–2307

That sounds reasonable. However, someone could theoretically use a C++20 compiler (aka clang 9) without concept support and that would lead to <type_traits> not compiling.

On the other hand I think that is a very bad use case that should not be suported. Maintainers thoughts?

cjdb added inline comments.Feb 12 2020, 2:33 PM
libcxx/include/type_traits
2306–2307

Since common_reference is really only useful with ranges in mind, we could section this code off with feature-test macros?

Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 8 2020, 11:41 AM
cjdb added a comment.Feb 7 2021, 7:09 PM

@miscco What is the status of this commit? I will review it on Wednesday if it's still in good shape.

Ping

@miscco What is the status of this commit? I will review it on Wednesday if it's still in good shape.

Hey,

as far as I can tell it should be still up to date. I have only limited time right now to look at it in depth though

ldionne added inline comments.Feb 10 2021, 12:04 PM
libcxx/include/type_traits
2306–2307

On the other hand I think that is a very bad use case that should not be suported. Maintainers thoughts?

I think it's entirely fine to use requires clauses if it simplifies the implementation. I have little sympathy for someone trying to use this with a compiler that doesn't support concepts, or with a compiler that does but with concepts disabled.

cjdb added a comment.Feb 17 2021, 1:21 PM

@miscco I took this patch and reworked it slightly in D96657 so that it's a bit closer to the standard wording (it's a separate patch because I didn't want to overwrite your implementation with such a drastic change).

Are you okay with this, or have I overlooked an advantage to you moving some of COMMON-REF into common_reference?

miscco abandoned this revision.Feb 19 2021, 9:46 AM

@cjdb I am totally fine with dropping this revision. Thanks a lot for working on this!