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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added subscribers: zoecarver, ldionne.
Quuxplusone added a comment.
This revision now requires changes to proceed.

I advise waiting for more opinions (e.g. @ldionne, @zoecarver) as to whether this is even worth pursuing at all.



================
Comment at: libcxx/include/__concepts/complete.h:22-25
+template <class _Tp>
+concept __complete = requires {
+  sizeof(_Tp);
+};
----------------
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.)


================
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>>>;
----------------
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.


================
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.");
       }
----------------
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).)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108645