[libcxx] [NFCI] Complete object type helper concept

[jloser]

Summary

In defining some iterator or ranges concepts, the notion of detecting
whether a type is complete or not is useful. Pull out this idea into a
helper concept: __complete_object_type and apply it in <__ranges/access.h>
and <__iterator/concepts.h>.

[• Quuxplusone]

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

libcxx/include/__concepts/complete.h

22–25

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.)

libcxx/include/__iterator/concepts.h

178

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.

libcxx/include/__ranges/access.h

67–73

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).)

[ldionne]

I think the value of this patch highly depends on whether we're right to look at the completeness of the type in contiguous_iterator. If not, then there's no duplication and I don't think this is worth pursuing. If so, then we can go forward but I'd find a different name too or try to make the concept really answer the question "is this type complete?", as Arthur mentioned.

libcxx/include/__iterator/concepts.h

178

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?

[jloser]

In D108645#2963230, @ldionne wrote:

I think the value of this patch highly depends on whether we're right to look at the completeness of the type in contiguous_iterator. If not, then there's no duplication and I don't think this is worth pursuing. If so, then we can go forward but I'd find a different name too or try to make the concept really answer the question "is this type complete?", as Arthur mentioned.

Agreed. I'm going to wait to hear back from @zoecarver or @cjdb if we should remove the complete use in __iterator/concepts.h.

If it's correct to only look at complete types in contiguous_iterator, then I'll clean up the concept naming.

[cjdb]

libcxx/include/__concepts/complete.h

22–25

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.

libcxx/include/__iterator/concepts.h

178

I think it might have been a hack to support something the library was failing to do correctly and ended up in development hell.

libcxx/include/__ranges/access.h

67–73

It's very likely that this is based on old wording. We can change it, but it should be done in its own commit.

[jloser]

Rename __complete concept to __complete_object_type.
Revert whitespace changes.

[jloser]

Abandoning this in favor of pursuing D108855. After D108855, there won't be any duplication with the complete_object_type concept.

libcxx/include/CMakeLists.txt

set(files
   __compare/compare_three_way_result.h
   __compare/ordering.h
   __concepts/arithmetic.h
   __concepts/assignable.h
   __concepts/boolean_testable.h
   __concepts/class_or_enum.h
   __concepts/common_reference_with.h
   __concepts/common_with.h
+  __concepts/complete.h
   __concepts/constructible.h
   __concepts/convertible_to.h
   __concepts/copyable.h
   __concepts/derived_from.h
   __concepts/destructible.h
   __concepts/different_from.h
   __concepts/equality_comparable.h
   __concepts/invocable.h

libcxx/include/__concepts/complete.h

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef _LIBCPP___CONCEPTS_COMPLETE_H
+#define _LIBCPP___CONCEPTS_COMPLETE_H
+
+#include <__config>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#pragma GCC system_header
+#endif
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_CONCEPTS)
+
+template <class _Tp>
+concept __complete = requires {
+  sizeof(_Tp);
+};
+
+#endif // _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_CONCEPTS)
+
+_LIBCPP_END_NAMESPACE_STD
+
+#endif // _LIBCPP___CONCEPTS_COMPLETE_H

libcxx/include/__iterator/concepts.h

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP___ITERATOR_CONCEPTS_H
 #define _LIBCPP___ITERATOR_CONCEPTS_H
 
 #include <__config>
+#include <__concepts/complete.h>
 #include <__iterator/incrementable_traits.h>
 #include <__iterator/iter_move.h>
 #include <__iterator/iterator_traits.h>
 #include <__iterator/readable_traits.h>
 #include <__memory/pointer_traits.h>
 #include <__utility/forward.h>
 #include <concepts>
 #include <type_traits>
requires(_Ip __i, const _Ip __j, const iter_difference_t<_Ip> __n) {
     { __j -  __n } -> same_as<_Ip>;
     {  __j[__n]  } -> same_as<iter_reference_t<_Ip>>;
   };
 
 template<class _Ip>
 concept contiguous_iterator =
   random_access_iterator<_Ip> &&
   derived_from<_ITER_CONCEPT<_Ip>, contiguous_iterator_tag> &&
-  is_lvalue_reference_v<iter_reference_t<_Ip>> &&
-  same_as<iter_value_t<_Ip>, remove_cvref_t<iter_reference_t<_Ip>>> &&
-  (is_pointer_v<_Ip> || requires { sizeof(__pointer_traits_element_type<_Ip>); }) &&
-  requires(const _Ip& __i) {
+  is_lvalue_reference_v<iter_reference_t<_Ip> > &&
+  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>>>;
   };
 
 template<class _Ip>
 concept __has_arrow = input_iterator<_Ip> && (is_pointer_v<_Ip> || requires(_Ip __i) { __i.operator->(); });
 
 // [indirectcallable.indirectinvocable]
 template<class _Fp, class _It>

libcxx/include/__ranges/access.h

 // -*- C++ -*-
 //===------------------------ __ranges/access.h ---------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #ifndef _LIBCPP___RANGES_ACCESS_H
 #define _LIBCPP___RANGES_ACCESS_H
 
 #include <__config>
+#include <__concepts/complete.h>
 #include <__iterator/concepts.h>
 #include <__iterator/readable_traits.h>
 #include <__ranges/enable_borrowed_range.h>
 #include <__utility/__decay_copy.h>
 #include <__utility/as_const.h>
 #include <__utility/forward.h>
 #include <concepts>
 #include <type_traits>

 #if !defined(_LIBCPP_HAS_NO_RANGES)
 
 // clang-format off
 
 namespace ranges {
   template <class _Tp>
   concept __can_borrow =
       is_lvalue_reference_v<_Tp> || enable_borrowed_range<remove_cvref_t<_Tp> >;
-
-  template<class _Tp>
-  concept __is_complete = requires { sizeof(_Tp); };
 } // namespace ranges
 
 // [range.access.begin]
 namespace ranges::__begin {
   template <class _Tp>
   concept __member_begin =
     __can_borrow<_Tp> &&
     requires(_Tp&& __t) {
concept __unqualified_begin =
     requires(_Tp && __t) {
       { _VSTD::__decay_copy(begin(__t)) } -> input_or_output_iterator;
     };
 
   struct __fn {
     template <class _Tp>
     requires is_array_v<remove_cv_t<_Tp>>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp& __t) const noexcept {
-      constexpr bool __complete = __is_complete<iter_value_t<_Tp> >;
-      if constexpr (__complete) { // used to disable cryptic diagnostic
+      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.");
       }
     }
 
     template <class _Tp>
     requires __member_begin<_Tp>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t) const
     noexcept(noexcept(_VSTD::__decay_copy(__t.begin())))
     {
       return __t.begin();
requires(_Tp && __t) {
       typename iterator_t<_Tp>;
       { _VSTD::__decay_copy(end(_VSTD::forward<_Tp>(__t))) } -> sentinel_for<iterator_t<_Tp> >;
     };
 
   class __fn {
   public:
     template <class _Tp, size_t _Np>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp (&__t)[_Np]) const noexcept {
-      constexpr bool __complete = __is_complete<remove_cv_t<_Tp> >;
-      if constexpr (__complete) { // used to disable cryptic diagnostic
+      constexpr bool __is_complete = __complete<remove_cv_t<_Tp> >;
+      if constexpr (__is_complete) { // used to disable cryptic diagnostic
         return __t + _Np;
       }
       else {
-        static_assert(__complete, "`std::ranges::end` is SFINAE-unfriendly on arrays of an incomplete type.");
+        static_assert(__is_complete, "`std::ranges::end` is SFINAE-unfriendly on arrays of an incomplete type.");
       }
     }
 
     template <class _Tp>
     requires __member_end<_Tp>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t) const
     noexcept(noexcept(_VSTD::__decay_copy(__t.end())))
     {

libcxx/include/module.modulemap

module concepts {
 
     module __concepts {
       module arithmetic                 { private header "__concepts/arithmetic.h"                }
       module assignable                 { private header "__concepts/assignable.h"                }
       module boolean_testable           { private header "__concepts/boolean_testable.h"          }
       module class_or_enum              { private header "__concepts/class_or_enum.h"             }
       module common_reference_with      { private header "__concepts/common_reference_with.h"     }
       module common_with                { private header "__concepts/common_with.h"               }
+      module complete                   { private header "__concepts/complete.h"                  }
       module constructible              { private header "__concepts/constructible.h"             }
       module convertible_to             { private header "__concepts/convertible_to.h"            }
       module copyable                   { private header "__concepts/copyable.h"                  }
       module derived_from               { private header "__concepts/derived_from.h"              }
       module destructible               { private header "__concepts/destructible.h"              }
       module different_from             { private header "__concepts/different_from.h"            }
       module equality_comparable        { private header "__concepts/equality_comparable.h"       }
       module invocable                  { private header "__concepts/invocable.h"                 }

libcxx/test/libcxx/diagnostics/detail.headers/concepts/complete.module.verify.cpp

+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: modules-build
+
+// WARNING: This test was generated by 'generate_private_header_tests.py'
+// and should not be edited manually.
+
+// expected-error@*:* {{use of private header from outside its module: '___concepts/complete.h'}}
+#include <___concepts/complete.h>