[libcxx-commits] [PATCH] D108645: [libcxx] [NFCI] Complete helper concept

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/include/__concepts/complete.h:22-25
+template <class _Tp>
+concept __complete = requires {
+  sizeof(_Tp);
+};
----------------
Quuxplusone wrote:
> I'm reasonably OK with this refactoring, but I'm also OK with the status quo; the cost here is small and the benefit is also small.
> However, I //do// think that if you're pulling this out into its own file, it needs to have a name that reflects its semantics. The concept is satisfied by complete object types and references-to-complete-object-types, and it is unsatisfied by
> - function types and reference-to-function types
> - incomplete types (cv void, incomplete object types, arrays of unknown bound)
> - anything else I missed?
> 
> Would it be reasonable to name this concept `__complete_object_type`?
> 
> However however, notice that this //is// an observable behavior change in the library, because atomic constraint satisfaction can be cached by the compiler. https://godbolt.org/z/nr3dPY7va "Completeness" is not really something that can be tested by a concept, because it's not invariant over the lifetime of a type in the type system. So really, any library facility that tries to constrain on completeness is already in a state of sin. (Does this mean "it doesn't matter, so leave it alone" or "it doesn't matter, so mess with it all you want"? I don't have a strong opinion, although I'm always inclined to the status quo.)
> function types and reference-to-function types

What's the motivation here?

> Would it be reasonable to name this concept `__complete_object_type`?

Seems fine to me. Could also be `__complete_class_type`.

> However however, notice that this //is// an observable behavior change in the library, because atomic constraint satisfaction can be cached by the compiler. https://godbolt.org/z/nr3dPY7va "Completeness" is not really something that can be tested by a concept, because it's not invariant over the lifetime of a type in the type system. So really, any library facility that tries to constrain on completeness is already in a state of sin. (Does this mean "it doesn't matter, so leave it alone" or "it doesn't matter, so mess with it all you want"? I don't have a strong opinion, although I'm always inclined to the status quo.)

It basically means "don't use the concept" unless your type is complete, for some definition of complete. We can certainly introduce atomicness into this, if it's necessary, but I'm not convinced it is.


================
Comment at: libcxx/include/__iterator/concepts.h:178
+  same_as<iter_value_t<_Ip>, remove_cvref_t<iter_reference_t<_Ip> >> &&
+  (is_pointer_v<_Ip> || __complete<__pointer_traits_element_type<_Ip> >) && requires(const _Ip& __i) {
     { _VSTD::to_address(__i) } -> same_as<add_pointer_t<iter_reference_t<_Ip>>>;
----------------
ldionne wrote:
> Quuxplusone wrote:
> > Remove `(is_pointer_v<_Ip> || __complete<__pointer_traits_element_type<_Ip> >) &&`; this is not part of the current working draft's wording.
> > https://eel.is/c++draft/iterator.concept.contiguous
> > 
> > Also, please revert the `>>`-to-`> >` whitespace changes. I know clang-format is to blame. Solution: don't trust (or listen to, or even run) clang-format.
> Indeed, I'm not sure why we even have that -- I think it should be removed. @zoecarver do you remember? I didn't review that specific patch. Maybe @cjdb knows too?
I think it might have been a hack to support something the library was failing to do correctly and ended up in development hell.


================
Comment at: libcxx/include/__ranges/access.h:67-73
+      constexpr bool __is_complete = __complete<iter_value_t<_Tp> >;
+      if constexpr (__is_complete) { // used to disable cryptic diagnostic
         return __t + 0;
       }
       else {
-        static_assert(__complete, "`std::ranges::begin` is SFINAE-unfriendly on arrays of an incomplete type.");
+        static_assert(__is_complete, "`std::ranges::begin` is SFINAE-unfriendly on arrays of an incomplete type.");
       }
----------------
Quuxplusone wrote:
> Here it seems we should be testing `remove_all_extents_t<_Tp>`, not `iter_value_t<_Tp>`.
> https://eel.is/c++draft/range.access#begin-2.2
> 
> (However, I can't make a test case where it matters. If `remove_all_extents_t<_Tp>` is incomplete, then naturally `iter_value_t<_Tp>` is also incomplete. Vice versa, if `iter_value_t<_Tp>` is incomplete, then it can only be because `remove_all_extents_t<_Tp>` is incomplete (since you can't have an array of arrays-of-unknown-bound, and you can't have an array of cv void).)
It's very likely that this is based on old wording. We can change it, but it should be done in its own commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108645