This is an archive of the discontinued LLVM Phabricator instance.

[libc++] P0898R3 2 of 12: Implement <concepts> header
AbandonedPublic

Authored by CaseyCarter on Jul 9 2018, 7:11 PM.

Details

Summary

Piece number two contains the header, a bunch of "add a new header" changes, and LIT config to grok tests that need concepts (Thanks to @EricWF's similar handling for coroutines which I copied shamelessly).

No tests yet, but fear not - there are 10 pieces yet to come ;)

Diff Detail

Event Timeline

CaseyCarter created this revision.Jul 9 2018, 7:11 PM
CaseyCarter added inline comments.Jul 9 2018, 7:24 PM
include/concepts
194

I suppose it would have been a good idea to fix this before submitting the patch for review - but it's too late now.

The issue here is that ConvertibleTo as specified rejects all conversions from array and function types. While this makes sense on the one hand - there are no expressions of array or function type from which to convert - it is a difference relative to is_convertible that I don't think any of the LEWG/LWG design discussions considered.

This conditional should be either removed, or the condition "flipped" so the concept definition agrees with the working draft. The affected test cases need to be conditional as well.

Quuxplusone added inline comments.
include/concepts
176

Peanut gallery asks: From lines 166-171 it looks awfully like __same_impl<_Tp, _Up> is true if and only if __same_impl<_Up, _Tp> is true. So why bother instantiating both templates?

281

Peanut gallery notices the lack of Constructible<_Tp, const _Tp&&> here, and assumes it's intentional?

296

Peanut gallery asks: Am I correct that the instances of ConvertibleTo<..., bool> here could (according to the WD) be replaced with -> bool? Am I correct that the instances of Same<..., bool> here could eventually be replaced with -> Same<bool> once they fix that issue you wrote a paper about?

I assume they're written like this for portability reasons.

380

Does this need a _VSTD:: for ADL reasons? (There's one in <tuple> that does use _VSTD::; there's two in <variant> that don't.)

395

I'm fairly confident that const remove_reference_t<_Tp>& is just const _Tp&.

CaseyCarter added inline comments.Jul 9 2018, 8:17 PM
include/concepts
176

This is the library implementation of the ["Same<T, U> subsumes Same<U, T>" requirement](http://eel.is/c++draft/concept.same#1).

281

Yes: Constructible<T, Args..> is equal to is_constructible_v<T, Args...> which is true iff the declaration T t(declval<Args>()...); is valid. Since decltype(declval<T>()) is add_rvalue_reference_t<T>, Constructible<_Tp, const _Tp> is equal to Constructible<_Tp, const _Tp&&>.

Admittedly the compiler doesn't know that - the subsumption rules don't know how declval is implemented - but no one has yet presented a use case where it would be helpful. If and when that happens, we can consider adding more terms to the concepts.

296

ConvertibleTo requires both implicit and explicit conversions to be valid and have equal results - so it's stronger than is_convertible and therefore the core language's notion of implicit convertibility. Consequently -> bool would not be equivalent.

Yes, P1084 will let us simplify all of these horrible nested requirements that duplicate the expressions. E; requires Concept<decltype(E), Args..>; will become { E } -> Concept<Args...>;.

380

Yes it does. Despite that the function name is ugly and a user can't hijack it, ADL will do crazy things like instantiating T when passed T* to determine T's associated namespaces.

395

Not when _Tp is a reference type: https://godbolt.org/g/Q2Hon8.

include/concepts
176

Ah, so this and tangentially line 280 are both about fiddling with "subsumption." Admittedly I don't know how the compiler side is implemented, but it seems too bad that the library has to do double the work (even at this leafy of a level) just to deal with subsumption, which AIUI is required in order to properly order competing template specializations (a situation that is(?) unlikely to come up in the average end-user's code). :/
Anyway, carry on.

395

Ah, right, because of the const. Never mind!

Qualify call to __invoke_constexpr in the definition of the Invocable concept.

CaseyCarter marked an inline comment as done.Jul 10 2018, 6:46 AM
CaseyCarter added inline comments.Jul 11 2018, 7:53 AM
include/concepts
152

I've just realized I failed to update <version> with the feature test macro value. Does libc++ have a plan yet for how to handle the feature test macros? Should I #define it in <version> and include that here?

I suspect we should rename __config to <version> and define all feature test macros therein.

176

Ideally, Same would be implemented with an intrinsic that tells the compiler to ignore the parameter mappings (http://eel.is/c++draft/temp.constr.atomic#2) when it checks the atomic constraints defined in this concept for equivalence. Or better yet, we could expose that feature in the language - say with a context-sensitive keyword concept symmetric Same - that could be applied to any two-type concept. There are quite a few symmetric concepts that could benefit similarly - Common, CommonReference, all of the FooWith concepts - for which we don't require subsumption because evaluating them is significantly more expensive than Same.

CaseyCarter abandoned this revision.May 4 2020, 11:05 AM