[libcxx][ranges] removes default_initializable from weakly_incrementable and view

[cjdb]

Summary

also:

• removes default constructors from predefined iterators
• makes span and string_view views

Partially implements P2325.
Partially resolves LWG3326.

[• Quuxplusone]

libcxx/include/span

25–27

Please linebreak for readability:

template<class ElementType, size_t Extent>
  inline constexpr bool ranges::enable_view<span<ElementType, Extent>> =
    (Extent == 0 || Extent == dynamic_extent);

536–537

Ditto here:

template<class _Tp, size_t _Extent>
inline constexpr bool ranges::enable_view<span<_Tp, _Extent>> =
  (_Extent == 0 || _Extent == dynamic_extent);

libcxx/include/string_view

188–189

I would like to see libc++ do the same thing consistently with enable_borrowed_range.h and enable_view.h. I admit, the places that want to specialize enable_view might not save very much (if anything) by avoiding <concepts>, but I think it's worth it to take a consistent approach to these "enable/disable" traits.

This might not be a blocker, since I can fiddle with organization after the fact.

...However, do I read correctly that you are moving ranges::view outside of <__ranges/view.h>? If that was intentional, then probably you should just rename <__ranges/view.h> to <__ranges/enable_view.h>, and that'll fix both complaints.

[cjdb]

rebases to activate CI

[ldionne]

What part of the LWG issue are we missing? Is it only the split_view part? If so, I think it's fine to mark it as done since it'll be handled when we implement split_view (http://eel.is/c++draft/range.split.view has the LWG issue applied).

libcxx/test/std/containers/views/range_concept_conformance.compile.pass.cpp

26

Missing space between > and && (elsewhere too).

[• Quuxplusone]

LGTM % comments.

libcxx/include/CMakeLists.txt

52–53 ↗

(On Diff #351077)

Alphabetize plz

libcxx/include/__ranges/concepts.h

13

Alphabetize plz

libcxx/include/__ranges/view.h

10–11

Update plz

libcxx/include/span

536–537

bump

libcxx/test/std/strings/string.view/range_concept_conformance.compile.pass.cpp

25

Missing space between > and && (elsewhere too).

namely, here

[cjdb]

[ http://wg21.link/P2325 | P2325 ] was approved by plenary and made the original version of this patch semi-wrong, so I repurposed the entire patch to deal with P2325 as well.

Due to the excessive changes, existing LGTMs need to be reapproved.

[• Quuxplusone]

If we're doing p2325 here now, then this patch should include changes to ostream_iterator and so on. (I think it would have been better to ship the LGTM'ed patch and then work on p2325 in a new PR; is that still an option, or is the old code lost now?)

[ldionne]

We'll also need to remember to bump the __cpp_lib_ranges feature-test macro to the right version when we do that (which I assume is when we'll have finished Ranges since we haven't discussed it).

Also, as discussed offline, please make the enable_view.h move in a separate NFC commit to reduce noise.

In D102468#2810753, @Quuxplusone wrote:

If we're doing p2325 here now, then this patch should include changes to ostream_iterator and so on. (I think it would have been better to ship the LGTM'ed patch and then work on p2325 in a new PR; is that still an option, or is the old code lost now?)

The patch I'm looking at right now does include the required changes to ostream_iterator, unless you're talking about other changes I'm not seeing?

libcxx/include/iterator

919 ↗

(On Diff #351189)

Why a FIXME instead of #if _LIBCPP_STD_VER > XXXX?

If both types exist, then they should be the same, so I don't see any sort of ABI issue in case that was the concern.

[• Quuxplusone]

LGTM modulo comments.
(My previous round of comments went in just before the update to include ostream_iterator et al.)

libcxx/docs/Cxx2aStatusPaperStatus.csv

196 ↗

(On Diff #351189)

@ldionne: What is the process by which papers get added here? Can we get someone to add all the papers that were adopted in this week's plenary? (Then I could check whether D97742 is finally unblocked! ;))

libcxx/include/iterator

919 ↗

(On Diff #351189)

I agree with Louis's assessment here.

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.winc/subsumption.verify.cpp

33 ↗

(On Diff #351189)

What is this test supposed to be testing?

libcxx/test/std/ranges/range.req/range.view/view.subsumption.verify.cpp

55–65 ↗

(On Diff #351189)

This test is now wrong. Please remove lines 55-65, and undo the file move.

[cjdb]

libcxx/include/iterator

919 ↗

(On Diff #351189)

I want to put this in a more relevant patch.

[cjdb]

In D102468#2810855, @Quuxplusone wrote:

LGTM modulo comments.
(My previous round of comments went in just before the update to include ostream_iterator et al.)

Not sure what you mean here. I transformed this patch in one step. If there's something I've missed, it's always been missed, so please let me know if that's the case.

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.winc/subsumption.verify.cpp

33 ↗

(On Diff #351189)

It's gone from a subsumption confirmation test to a subsumption regression test, making sure that weakly_incrementable doesn't subsume default_initializable.

libcxx/test/std/ranges/range.req/range.view/view.subsumption.verify.cpp

55–65 ↗

(On Diff #351189)

Why is it wrong?

[cjdb]

libcxx/test/std/ranges/range.req/range.view/view.subsumption.verify.cpp

55–65 ↗

(On Diff #351189)

Don't worry, I understand now (for future readers: the test is not necessary since the change on libcxx/test/std/ranges/range.req/range.view/view.compile.pass.cpp:49 acts as the regression test). This applies to libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.winc/subsumption.verify.cpp:33 as well.

@Quuxplusone In future, could you please explain why you think something is wrong instead of matter-of-factly stating it? I had no context, and from what I could see, the test looked correct to me.

[cjdb]

responds to feedback

[cjdb]

removes extraneous header

[cjdb]

inconsequential changes

[zoecarver]

Let's land this to unblock the other patches.

libcxx/include/iterator

919 ↗

(On Diff #351189)

Theoretically there's a section in the ranges status document to track things like this... but I guess we don't really use it.

Anyway, please make sure you have this on some todo list that you're going to look at at some point (or put it in the ranges status doc). Make sure it's not just a browser tab ;)

libcxx/include/string_view

662

Why are we putting this change in this patch. I feel like this should be its own patch.

[ldionne]

Thanks! Now we'll be able to update all the in-progress views to this paper.

libcxx/docs/Cxx2aStatusPaperStatus.csv

196 ↗

(On Diff #351189)

Back in the days, Marshall would keep those lists up to date. More recently, @curdeius has been doing a lot of it. I'll update the list with Monday's plenary.

[ldionne]

I missed that un-applied comment in my LGTM a second ago. Still LGTM, but why not fix the FIXME now?

libcxx/include/iterator

919 ↗

(On Diff #351189)

Actually, I think this should be

#if _LIBCPP_SUPPORTS_RANGES
ranges::iterator_t<Container> iter;
#else
 typename _Container::iterator iter;
#endif

I don't see a reason to wait for another patch?

[ldionne]

libcxx/include/string_view

662

Technically, wg21.link/P2325 does change that specialization as this patch does it. I think it's fine to do both in this patch, even though it could have indeed be tackled as a separate patch that implements only LWG3326. I think it's fine as-is.

[ldionne]

Ship-it. I'll be looking forward to a patch that makes insert_iterator up-to-spec for C++20.

libcxx/include/iterator

919 ↗

(On Diff #351189)

As discussed offline, this will be tackled in a separate patch that makes insert_iterator fully up-to-spec for C++20.

[cjdb]

rebases for a successful CI

libcxx/docs/Cxx2aStatusIssuesStatus.csv

 "`3317 <https://wg21.link/LWG3317>`__","Incorrect ``operator<<``\  for floating-point durations","Prague","",""
 "`3318 <https://wg21.link/LWG3318>`__","Clarify whether clocks can represent time before their epoch","Prague","",""
 "`3319 <https://wg21.link/LWG3319>`__","Properly reference specification of IANA time zone database","Prague","",""
 "`3320 <https://wg21.link/LWG3320>`__","``span::cbegin/cend``\  methods produce different results than ``std::[ranges::]cbegin/cend``\ ","Prague","|Complete|",""
 "`3321 <https://wg21.link/LWG3321>`__","``uninitialized_construct_using_allocator``\  should use ``construct_at``\ ","Prague","",""
 "`3323 <https://wg21.link/LWG3323>`__","``*has-tuple-element*``\  helper concept needs ``convertible_to``\ ","Prague","",""
 "`3324 <https://wg21.link/LWG3324>`__","Special-case ``std::strong/weak/partial_order``\  for pointers","Prague","",""
 "`3325 <https://wg21.link/LWG3325>`__","Constrain return type of transformation function for ``transform_view``\ ","Prague","",""
-"`3326 <https://wg21.link/LWG3326>`__","``enable_view``\  has false positives","Prague","",""
+"`3326 <https://wg21.link/LWG3326>`__","``enable_view``\  has false positives","Prague","|In progress|",""
 "`3327 <https://wg21.link/LWG3327>`__","Format alignment specifiers vs. text direction","Prague","",""
 "`3328 <https://wg21.link/LWG3328>`__","Clarify that ``std::string``\  is not good for UTF-8","Prague","",""
 "`3329 <https://wg21.link/LWG3329>`__","``totally_ordered_with``\  both directly and indirectly requires ``common_reference_with``\ ","Prague","|Complete|","13.0"
 "`3330 <https://wg21.link/LWG3330>`__","Include ``<compare>``\  from most library headers","Prague","",""
 "`3331 <https://wg21.link/LWG3331>`__","Define ``totally_ordered/_with``\  in terms of ``*partially-ordered-with*``\ ","Prague","|Complete|","13.0"
 "`3332 <https://wg21.link/LWG3332>`__","Issue in |sect|\ [time.format]","Prague","",""
 "`3334 <https://wg21.link/LWG3334>`__","``basic_osyncstream``\  move assignment and destruction calls ``basic_syncbuf::emit()``\  twice","Prague","",""
 "`3335 <https://wg21.link/LWG3335>`__","Resolve C++20 NB comments US 273 and GB 274","Prague","",""

libcxx/include/__ranges/concepts.h

 // -*- C++ -*-
 //===--------------------- __ranges/concepts.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_CONCEPTS_H
 #define _LIBCPP_RANGES_CONCEPTS_H
 
 #include <__config>
+#include <__iterator/concepts.h>
+#include <__ranges/access.h>
+#include <__ranges/view.h>
 #include <type_traits>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_PUSH_MACROS
 #include <__undef_macros>
namespace ranges {
   using range_value_t = iter_value_t<iterator_t<_Rp> >;
 
   template <range _Rp>
   using range_reference_t = iter_reference_t<iterator_t<_Rp> >;
 
   template <range _Rp>
   using range_rvalue_reference_t = iter_rvalue_reference_t<iterator_t<_Rp> >;
 
+  // [range.view], views
+
+  // `enable_view` defined in <__ranges/view.h>
+  // `view_base` defined in <__ranges/view.h>
+
+  template <class _Tp>
+  concept view =
+    range<_Tp> &&
+    movable<_Tp> &&
+    default_initializable<_Tp> &&
+    enable_view<_Tp>;
+
   // [range.refinements], other range refinements
   template <class _Tp>
   concept input_range = range<_Tp> && input_iterator<iterator_t<_Tp> >;
 
   template <class _Tp>
   concept forward_range = input_range<_Tp> && forward_iterator<iterator_t<_Tp> >;
 
   template <class _Tp>

libcxx/include/__ranges/view.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___RANGES_VIEW_H
 #define _LIBCPP___RANGES_VIEW_H
 
 #include <__config>
-#include <__ranges/concepts.h>
 #include <concepts>
 
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_PUSH_MACROS
 #include <__undef_macros>
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 #if !defined(_LIBCPP_HAS_NO_RANGES)
 
 namespace ranges {
 
 struct view_base { };
 
 template <class _Tp>
 inline constexpr bool enable_view = derived_from<_Tp, view_base>;
 
-template <class _Tp>
-concept view =
-  range<_Tp> &&
-  movable<_Tp> &&
-  default_initializable<_Tp> &&
-  enable_view<_Tp>;
-
 } // end namespace ranges
 
 #endif // !_LIBCPP_HAS_NO_RANGES
 
 _LIBCPP_END_NAMESPACE_STD
 
 _LIBCPP_POP_MACROS
 
 #endif // _LIBCPP___RANGES_VIEW_H

libcxx/include/span

 
 // constants
 inline constexpr size_t dynamic_extent = numeric_limits<size_t>::max();
 
 // [views.span], class template span
 template <class ElementType, size_t Extent = dynamic_extent>
     class span;
 
 template<class ElementType, size_t Extent>
+  inline constexpr bool ranges::enable_view<span<ElementType, Extent>> = Extent == 0 ||
+    Extent == dynamic_extent;
+
+template<class ElementType, size_t Extent>
     inline constexpr bool ranges::enable_borrowed_range<span<ElementType, Extent>> = true;
 
 // [span.objectrep], views of object representation
 template <class ElementType, size_t Extent>
     span<const byte, ((Extent == dynamic_extent) ? dynamic_extent :
         (sizeof(ElementType) * Extent))> as_bytes(span<ElementType, Extent> s) noexcept;
 
 template <class ElementType, size_t Extent>
template<class Container>
     span(const Container&) -> span<const typename Container::value_type>;
 
 } // namespace std
 
 */
 
 #include <__config>
 #include <__ranges/enable_borrowed_range.h>
+#include <__ranges/view.h>
 #include <array>        // for array
 #include <cstddef>      // for byte
 #include <iterator>     // for iterators
 #include <type_traits>  // for remove_cv, etc
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
// [span.iter], span iterator support
     { return {reinterpret_cast<byte *>(data()), size_bytes()}; }
 
 private:
     pointer   __data;
     size_type __size;
 };
 
 #if !defined(_LIBCPP_HAS_NO_RANGES)
+template <class _ElementType, size_t _Extent>
+inline constexpr bool ranges::enable_view<span<_ElementType, _Extent>> = _Extent == 0 || _Extent == dynamic_extent;
+
 template <class _Tp, size_t _Extent>
 inline constexpr bool ranges::enable_borrowed_range<span<_Tp, _Extent> > = true;
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
 
 //  as_bytes & as_writable_bytes
 template <class _Tp, size_t _Extent>
 _LIBCPP_INLINE_VISIBILITY
 auto as_bytes(span<_Tp, _Extent> __s) noexcept

libcxx/include/string_view

 
 namespace std {
 
     // 7.2, Class template basic_string_view
     template<class charT, class traits = char_traits<charT>>
         class basic_string_view;
 
     template<class charT, class traits>
+    inline constexpr bool ranges::enable_view<basic_string_view<charT, traits>> = true;
+
+    template<class charT, class traits>
     inline constexpr bool ranges::enable_borrowed_range<basic_string_view<charT, traits>> = true;  // C++20
 
     // 7.9, basic_string_view non-member comparison functions
     template<class charT, class traits>
     constexpr bool operator==(basic_string_view<charT, traits> x,
                               basic_string_view<charT, traits> y) noexcept;
     template<class charT, class traits>
     constexpr bool operator!=(basic_string_view<charT, traits> x,
namespace std {
   constexpr basic_string_view<char32_t> operator "" sv( const char32_t *str, size_t len ) noexcept;
 
 }  // namespace std
 
 
 */
 
 #include <__config>
 #include <__ranges/enable_borrowed_range.h>
+#include <__ranges/view.h>
 #include <__string>
 #include <iosfwd>
 #include <algorithm>
 #include <compare>
 #include <iterator>
 #include <limits>
 #include <stdexcept>
 #include <version>

 #endif
 
 private:
     const   value_type* __data;
     size_type           __size;
 };
 
 #if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_RANGES)
-template <class _CharT, class _Traits>
+template<class _CharT, class _Traits>
+inline constexpr bool ranges::enable_view<basic_string_view<_CharT, _Traits>> = true;
+
+template <class _CharT, class _Traits>
 inline constexpr bool ranges::enable_borrowed_range<basic_string_view<_CharT, _Traits> > = true;
 #endif // _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_RANGES)
 
 // [string.view.comparison]
 // operator ==
 template<class _CharT, class _Traits>
 _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
 bool operator==(basic_string_view<_CharT, _Traits> __lhs,

libcxx/test/std/containers/views/range_concept_conformance.compile.pass.cpp

 #include <ranges>
 
 using range = std::span<int>;
 namespace stdr = std::ranges;
 
 static_assert(std::same_as<stdr::iterator_t<range>, range::iterator>);
 static_assert(stdr::common_range<range>);
 static_assert(stdr::random_access_range<range>);
-static_assert(!stdr::view<range>);
+static_assert(stdr::view<range> && stdr::enable_view<range>);
 
 static_assert(std::same_as<stdr::iterator_t<range const>, range::iterator>);
 static_assert(stdr::common_range<range const>);
 static_assert(stdr::random_access_range<range const>);
-static_assert(!stdr::view<range const>);
+static_assert(!stdr::view<range const> && !stdr::enable_view<range const>);

libcxx/test/std/strings/string.view/range_concept_conformance.compile.pass.cpp

 #include <concepts>
 #include <ranges>
 
 namespace stdr = std::ranges;
 
 static_assert(std::same_as<stdr::iterator_t<std::string_view>, std::string_view::iterator>);
 static_assert(stdr::common_range<std::string_view>);
 static_assert(stdr::random_access_range<std::string_view>);
-static_assert(!stdr::view<std::string_view>);
+static_assert(stdr::view<std::string_view> && stdr::enable_view<std::string_view>);
 
 static_assert(std::same_as<stdr::iterator_t<std::string_view const>, std::string_view::const_iterator>);
 static_assert(stdr::common_range<std::string_view const>);
 static_assert(stdr::random_access_range<std::string_view const>);
-static_assert(!stdr::view<std::string_view const>);
+static_assert(!stdr::view<std::string_view const> && !stdr::enable_view<std::string_view const>);