[libc++] Implement P2442R1 std::views::chunk#171234
Open
shyeyian wants to merge 44 commits into
Open
Conversation
llvmbot
added
the
libc++
Member
|
@llvm/pr-subscribers-libcxx Author: anonymous (anonymouspc) ChangesThis PR implements std::ranges::chunk_view and std::views::chunk in header <__ranges/chunk_view.h>. This is partial P2242R1 (as P2242R1 contains both views::chunk and views::slide). Thank you! Patch is 41.88 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/171234.diff 18 Files Affected:
diff --git a/libcxx/docs/FeatureTestMacroTable.rst b/libcxx/docs/FeatureTestMacroTable.rst
index 756bdf71f8b22..49672e5ccf70a 100644
--- a/libcxx/docs/FeatureTestMacroTable.rst
+++ b/libcxx/docs/FeatureTestMacroTable.rst
@@ -370,7 +370,7 @@ Status
---------------------------------------------------------- -----------------
``__cpp_lib_ranges_as_rvalue`` ``202207L``
---------------------------------------------------------- -----------------
- ``__cpp_lib_ranges_chunk`` *unimplemented*
+ ``__cpp_lib_ranges_chunk`` ``202202L``
---------------------------------------------------------- -----------------
``__cpp_lib_ranges_chunk_by`` ``202202L``
---------------------------------------------------------- -----------------
diff --git a/libcxx/docs/ReleaseNotes/22.rst b/libcxx/docs/ReleaseNotes/22.rst
index 9f1e3d570f254..78796f15f03f9 100644
--- a/libcxx/docs/ReleaseNotes/22.rst
+++ b/libcxx/docs/ReleaseNotes/22.rst
@@ -49,6 +49,7 @@ Implemented Papers
- P2835R7: Expose ``std::atomic_ref``'s object address (`Github <https://llvm.org/PR118377>`__)
- P2944R3: Comparisons for ``reference_wrapper`` (`Github <https://llvm.org/PR105424>`__)
- P3168R2: Give ``std::optional`` Range Support (`Github <https://llvm.org/PR105430>`__)
+- P2442R1: Add ``std::views::chunk`` (`Github <https://llvm.org/PR171232>`__)
Improvements and New Features
-----------------------------
diff --git a/libcxx/docs/Status/Cxx23Papers.csv b/libcxx/docs/Status/Cxx23Papers.csv
index b655384bad7f2..7aa2f07d9bb57 100644
--- a/libcxx/docs/Status/Cxx23Papers.csv
+++ b/libcxx/docs/Status/Cxx23Papers.csv
@@ -48,7 +48,7 @@
"`P2387R3 <https://wg21.link/P2387R3>`__","Pipe support for user-defined range adaptors","2022-02 (Virtual)","|Complete|","19","`#105183 <https://github.com/llvm/llvm-project/issues/105183>`__",""
"`P2440R1 <https://wg21.link/P2440R1>`__","``ranges::iota``, ``ranges::shift_left`` and ``ranges::shift_right``","2022-02 (Virtual)","|Partial|","","`#105184 <https://github.com/llvm/llvm-project/issues/105184>`__","Only ``ranges::iota`` is implemented."
"`P2441R2 <https://wg21.link/P2441R2>`__","``views::join_with``","2022-02 (Virtual)","|Complete|","21","`#105185 <https://github.com/llvm/llvm-project/issues/105185>`__",""
-"`P2442R1 <https://wg21.link/P2442R1>`__","Windowing range adaptors: ``views::chunk`` and ``views::slide``","2022-02 (Virtual)","","","`#105187 <https://github.com/llvm/llvm-project/issues/105187>`__",""
+"`P2442R1 <https://wg21.link/P2442R1>`__","Windowing range adaptors: ``views::chunk`` and ``views::slide``","2022-02 (Virtual)","|Partial|","22","`#105187 <https://github.com/llvm/llvm-project/issues/105187>`__","Only ``views::chunk`` is implemented."
"`P2443R1 <https://wg21.link/P2443R1>`__","``views::chunk_by``","2022-02 (Virtual)","|Complete|","18","`#105188 <https://github.com/llvm/llvm-project/issues/105188>`__",""
"","","","","","",""
"`P0009R18 <https://wg21.link/P0009R18>`__","mdspan: A Non-Owning Multidimensional Array Reference","2022-07 (Virtual)","|Complete|","18","`#105189 <https://github.com/llvm/llvm-project/issues/105189>`__",""
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index cbcd764e67d93..afe8391f1a5d5 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -703,6 +703,7 @@ set(files
__ranges/all.h
__ranges/as_rvalue_view.h
__ranges/chunk_by_view.h
+ __ranges/chunk_view.h
__ranges/common_view.h
__ranges/concepts.h
__ranges/container_compatible_range.h
diff --git a/libcxx/include/__cxx03/module.modulemap b/libcxx/include/__cxx03/module.modulemap
index 34a2d0f25fc45..b7eee575090ce 100644
--- a/libcxx/include/__cxx03/module.modulemap
+++ b/libcxx/include/__cxx03/module.modulemap
@@ -1701,6 +1701,7 @@ module cxx03_std_private_ranges_all [system] {
}
module cxx03_std_private_ranges_as_rvalue_view [system] { header "__ranges/as_rvalue_view.h" }
module cxx03_std_private_ranges_chunk_by_view [system] { header "__ranges/chunk_by_view.h" }
+module cxx03_std_private_ranges_chunk_view [system] { header "__ranges/chunk_view.h" }
module cxx03_std_private_ranges_common_view [system] { header "__ranges/common_view.h" }
module cxx03_std_private_ranges_concepts [system] {
header "__ranges/concepts.h"
diff --git a/libcxx/include/__ranges/chunk_view.h b/libcxx/include/__ranges/chunk_view.h
new file mode 100644
index 0000000000000..ae37c9784e081
--- /dev/null
+++ b/libcxx/include/__ranges/chunk_view.h
@@ -0,0 +1,544 @@
+// -*- 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_CHUNK_VIEW_H
+#define _LIBCPP___RANGES_CHUNK_VIEW_H
+
+#include <__algorithm/ranges_min.h>
+#include <__assert>
+#include <__concepts/constructible.h>
+#include <__concepts/convertible_to.h>
+#include <__config>
+#include <__functional/bind_back.h>
+#include <__iterator/advance.h>
+#include <__iterator/concepts.h>
+#include <__iterator/default_sentinel.h>
+#include <__iterator/distance.h>
+#include <__iterator/iter_move.h>
+#include <__iterator/iter_swap.h>
+#include <__iterator/iterator_traits.h>
+#include <__memory/addressof.h>
+#include <__ranges/access.h>
+#include <__ranges/all.h>
+#include <__ranges/concepts.h>
+#include <__ranges/enable_borrowed_range.h>
+#include <__ranges/non_propagating_cache.h>
+#include <__ranges/range_adaptor.h>
+#include <__ranges/take_view.h>
+#include <__ranges/view_interface.h>
+#include <__type_traits/conditional.h>
+#include <__type_traits/decay.h>
+#include <__type_traits/is_nothrow_constructible.h>
+#include <__type_traits/make_unsigned.h>
+#include <__utility/forward.h>
+#include <__utility/move.h>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+# pragma GCC system_header
+#endif
+
+_LIBCPP_PUSH_MACROS
+#include <__undef_macros>
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+#if _LIBCPP_STD_VER >= 23
+
+namespace ranges {
+
+template <class _Integral>
+inline _LIBCPP_HIDE_FROM_ABI constexpr auto __div_ceil(_Integral __num, _Integral __denom) {
+ _Integral __r = __num / __denom;
+ if (__num % __denom)
+ ++__r;
+ return __r;
+}
+
+template <view _View>
+ requires input_range<_View>
+class _LIBCPP_ABI_LLVM18_NO_UNIQUE_ADDRESS chunk_view : public view_interface<chunk_view<_View>> {
+ _LIBCPP_NO_UNIQUE_ADDRESS _View __base_;
+ _LIBCPP_NO_UNIQUE_ADDRESS range_difference_t<_View> __n_;
+ _LIBCPP_NO_UNIQUE_ADDRESS range_difference_t<_View> __remainder_;
+ _LIBCPP_NO_UNIQUE_ADDRESS __non_propagating_cache<iterator_t<_View>> __current_;
+
+ class __outer_iterator;
+ class __inner_iterator;
+
+public:
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit chunk_view(_View __base, range_difference_t<_View> __n)
+ : __base_(std::move(__base)), __n_(__n), __remainder_(0) {
+ _LIBCPP_ASSERT_PEDANTIC(__n > 0, "Trying to construct a chunk_view with chunk size <= 0");
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr _View base() const&
+ requires std::copy_constructible<_View>
+ {
+ return __base_;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr _View base() && { return std::move(__base_); }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __outer_iterator begin() {
+ __current_.__emplace(ranges::begin(__base_));
+ __remainder_ = __n_;
+ return __outer_iterator(*this);
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr default_sentinel_t end() const noexcept { return std::default_sentinel; }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr auto size()
+ requires sized_range<_View>
+ {
+ return std::__to_unsigned_like(ranges::__div_ceil(ranges::distance(__base_), __n_));
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr auto size() const
+ requires sized_range<const _View>
+ {
+ return std::__to_unsigned_like(ranges::__div_ceil(ranges::distance(__base_), __n_));
+ }
+};
+
+template <view _View>
+ requires input_range<_View>
+class chunk_view<_View>::__outer_iterator {
+ friend chunk_view;
+
+ chunk_view* __parent_;
+
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __outer_iterator(chunk_view& __parent)
+ : __parent_(std::addressof(__parent)) {}
+
+public:
+ class value_type;
+ using iterator_concept = input_iterator_tag;
+ using difference_type = range_difference_t<_View>;
+
+ _LIBCPP_HIDE_FROM_ABI __outer_iterator(__outer_iterator&&) = default;
+
+ _LIBCPP_HIDE_FROM_ABI __outer_iterator& operator=(__outer_iterator&&) = default;
+
+ _LIBCPP_HIDE_FROM_ABI constexpr value_type operator*() const {
+ _LIBCPP_ASSERT_PEDANTIC(*this != default_sentinel, "Trying to dereference past-the-end chunk_view iterator.");
+ return value_type(*__parent_);
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __outer_iterator& operator++() {
+ ranges::advance(*__parent_->__current_, __parent_->__remainder_, ranges::end(__parent_->__base_));
+ __parent_->__remainder_ = __parent_->__n_;
+ return *this;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr void operator++(int) { ++*this; }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr bool operator==(const __outer_iterator& __i, default_sentinel_t) {
+ return *__i.__parent_->__current_ == ranges::end(__i.__parent_->__base_) && __i.__parent_->__remainder_ != 0;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr difference_type operator-(default_sentinel_t, const __outer_iterator& __i)
+ requires sized_sentinel_for<sentinel_t<_View>, iterator_t<_View>>
+ {
+ const auto __dist = ranges::end(__i.__parent_->__base_) - *__i.__parent_->__current_;
+ if (__dist < __i.__parent_->__remainder_)
+ return __dist == 0 ? 0 : 1;
+ return ranges::__div_ceil(__dist - __i.__parent_->__remainder_, __i.__parent_->__n_) + 1;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr difference_type operator-(const __outer_iterator& __i, default_sentinel_t __s)
+ requires sized_sentinel_for<sentinel_t<_View>, iterator_t<_View>>
+ {
+ return -(__s - __i);
+ }
+};
+
+template <view _View>
+ requires input_range<_View>
+class chunk_view<_View>::__outer_iterator::value_type : public view_interface<value_type> {
+ friend __outer_iterator;
+
+ chunk_view* __parent_;
+
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit value_type(chunk_view& __parent) : __parent_(std::addressof(__parent)) {}
+
+public:
+ _LIBCPP_HIDE_FROM_ABI constexpr __inner_iterator begin() const noexcept { return __inner_iterator(*__parent_); }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr default_sentinel_t end() const noexcept { return default_sentinel; }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr auto size() const
+ requires sized_sentinel_for<sentinel_t<_View>, iterator_t<_View>>
+ {
+ return std::__to_unsigned_like(
+ ranges::min(__parent_->__remainder_, ranges::end(__parent_->__base_) - *__parent_->__current_));
+ }
+};
+
+template <view _View>
+ requires input_range<_View>
+class chunk_view<_View>::__inner_iterator {
+ friend chunk_view;
+
+ chunk_view* __parent_;
+
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __inner_iterator(chunk_view& __parent)
+ : __parent_(std::addressof(__parent)) {}
+
+public:
+ using iterator_concept = input_iterator_tag;
+ using difference_type = range_difference_t<_View>;
+ using value_type = range_value_t<_View>;
+
+ _LIBCPP_HIDE_FROM_ABI __inner_iterator(__inner_iterator&&) = default;
+
+ _LIBCPP_HIDE_FROM_ABI __inner_iterator& operator=(__inner_iterator&&) = default;
+
+ _LIBCPP_HIDE_FROM_ABI constexpr const iterator_t<_View> base() const& { return *__parent_->__current_; }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr range_reference_t<_View> operator*() const {
+ _LIBCPP_ASSERT_PEDANTIC(*this != default_sentinel, "Trying to dereference past-the-end chunk_view iterator");
+ return **__parent_->__current_;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __inner_iterator& operator++() {
+ ++*__parent_->__current_;
+ if (*__parent_->__current_ == ranges::end(__parent_->__base_))
+ __parent_->__remainder_ = 0;
+ else
+ --__parent_->__remainder_;
+ return *this;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr void operator++(int) { ++*this; }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr bool operator==(const __inner_iterator& __i, default_sentinel_t) {
+ return __i.__parent_->__remainder_ == 0;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr difference_type operator-(default_sentinel_t, const __inner_iterator& __i)
+ requires sized_sentinel_for<sentinel_t<_View>, iterator_t<_View>>
+ {
+ return ranges::min(__i.__parent_->__remainder_, ranges::end(__i.__parent_->__base_) - *__i.__parent_->__current_);
+ }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr difference_type operator-(const __inner_iterator& __i, default_sentinel_t __s)
+ requires sized_sentinel_for<sentinel_t<_View>, iterator_t<_View>>
+ {
+ return -(__s - __i);
+ }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr auto
+ iter_move(const __inner_iterator& __i) noexcept(noexcept(ranges::iter_move(*__i.__parent_->__current_))) {
+ return ranges::iter_move(*__i.__parent_->__current_);
+ }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr void
+ iter_swap(const __inner_iterator& __x, const __inner_iterator& __y) noexcept(
+ noexcept((ranges::iter_swap(*__x.__parent_->__current_, *__y.__parent_->__current_))))
+ requires indirectly_swappable<iterator_t<_View>>
+ {
+ return ranges::iter_swap(*__x.__parent_->__current_, *__y.__parent_->__current_);
+ }
+};
+
+template <view _View>
+ requires forward_range<_View>
+class _LIBCPP_ABI_LLVM18_NO_UNIQUE_ADDRESS chunk_view<_View> : public view_interface<chunk_view<_View>> {
+ _LIBCPP_NO_UNIQUE_ADDRESS _View __base_;
+ _LIBCPP_NO_UNIQUE_ADDRESS range_difference_t<_View> __n_;
+
+ template <bool _Const>
+ class __iterator;
+
+public:
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit chunk_view(_View __base, range_difference_t<_View> __n)
+ : __base_(std::move(__base)), __n_(__n) {
+ _LIBCPP_ASSERT_PEDANTIC(__n > 0, "Trying to construct a chunk_view with chunk size <= 0");
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr _View base() const&
+ requires copy_constructible<_View>
+ {
+ return __base_;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr _View base() && { return std::move(__base_); }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr auto begin()
+ requires(!__simple_view<_View>)
+ {
+ return __iterator<false>(this, ranges::begin(__base_));
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr auto begin() const
+ requires forward_range<const _View>
+ {
+ return __iterator<true>(this, ranges::begin(__base_));
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr auto end()
+ requires(!__simple_view<_View>)
+ {
+ if constexpr (common_range<_View> && sized_range<_View>) {
+ auto __missing = (__n_ - ranges::distance(__base_) % __n_) % __n_;
+ return __iterator<false>(this, ranges::end(__base_), __missing);
+ } else if constexpr (common_range<_View> && !bidirectional_range<_View>)
+ return __iterator<false>(this, ranges::end(__base_));
+ else
+ return default_sentinel;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr auto end() const
+ requires forward_range<const _View>
+ {
+ if constexpr (common_range<const _View> && sized_range<const _View>) {
+ auto __missing = (__n_ - ranges::distance(__base_) % __n_) % __n_;
+ return __iterator<true>(this, ranges::end(__base_), __missing);
+ } else if constexpr (common_range<const _View> && !bidirectional_range<const _View>)
+ return __iterator<true>(this, ranges::end(__base_));
+ else
+ return default_sentinel;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr auto size()
+ requires sized_range<_View>
+ {
+ return std::__to_unsigned_like(ranges::__div_ceil(ranges::distance(__base_), __n_));
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr auto size() const
+ requires sized_range<const _View>
+ {
+ return std::__to_unsigned_like(ranges::__div_ceil(ranges::distance(__base_), __n_));
+ }
+};
+
+template <view _View>
+ requires forward_range<_View>
+template <bool _Const>
+class chunk_view<_View>::__iterator {
+ friend chunk_view;
+
+ using _Parent _LIBCPP_NODEBUG = __maybe_const<_Const, chunk_view>;
+ using _Base _LIBCPP_NODEBUG = __maybe_const<_Const, _View>;
+
+ iterator_t<_Base> __current_ = iterator_t<_Base>();
+ sentinel_t<_Base> __end_ = sentinel_t<_Base>();
+ range_difference_t<_Base> __n_ = 0;
+ range_difference_t<_Base> __missing_ = 0;
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __iterator(
+ _Parent* __parent, iterator_t<_Base> __current, range_difference_t<_Base> __missing = 0)
+ : __current_(__current), __end_(ranges::end(__parent->__base_)), __n_(__parent->__n_), __missing_(__missing) {}
+
+ static consteval auto __get_iterator_concept() {
+ if constexpr (random_access_range<_Base>)
+ return random_access_iterator_tag{};
+ else if constexpr (bidirectional_range<_Base>)
+ return bidirectional_iterator_tag{};
+ else
+ return forward_iterator_tag{};
+ }
+
+public:
+ using iterator_category = input_iterator_tag;
+ using iterator_concept = decltype(__iterator::__get_iterator_concept());
+ using value_type = decltype(views::take(subrange(__current_, __end_), __n_));
+ using difference_type = range_difference_t<_Base>;
+
+ _LIBCPP_HIDE_FROM_ABI __iterator() = default;
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __iterator(__iterator<!_Const> __i)
+ requires _Const && convertible_to<iterator_t<_View>, iterator_t<_Base>> &&
+ convertible_to<sentinel_t<_View>, sentinel_t<_Base>>
+ : __current_(std::move(__i.__current_)),
+ __end_(std::move(__i.__end_)),
+ __n_(__i.__n_),
+ __missing_(__i.__missing_) {}
+
+ _LIBCPP_HIDE_FROM_ABI constexpr iterator_t<_Base> base() const { return __current_; }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr value_type operator*() const {
+ _LIBCPP_ASSERT_PEDANTIC(__current_ != __end_, "Trying to dereference past-the-end chunk_view iterator");
+ return views::take(subrange(__current_, __end_), __n_);
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr value_type operator[](difference_type __pos) const
+ requires random_access_range<_Base>
+ {
+ return *(*this + __pos);
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __iterator& operator++() {
+ _LIBCPP_ASSERT_PEDANTIC(__current_ != __end_, "Trying to advance past-the-end chunk_view iterator");
+ __missing_ = ranges::advance(__current_, __n_, __end_);
+ return *this;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __iterator operator++(int) {
+ auto __tmp = *this;
+ ++*this;
+ return __tmp;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __iterator& operator--()
+ requires bidirectional_range<_Base>
+ {
+ ranges::advance(__current_, __missing_ - __n_);
+ __missing_ = 0;
+ return *this;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __iterator operator--(int) {
+ auto __tmp = *this;
+ --*this;
+ return __tmp;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __iterator& operator+=(difference_type __x)
+ requires random_access_range<_Base>
+ {
+ if (__x > 0) {
+ _LIBCPP_ASSERT_PEDANTIC(ranges::distance(__current_, __end_) > __n_ * (__x - 1),
+ "Trying to advance chunk_view iterator out of range");
+ ranges::advance(__current_, __n_ * (__x - 1));
+ __missing_ = ranges::advance(__current_, __n_, __end_);
+ } else if (__x < 0) {
+ ranges::advance(__current_, __n_ * __x + __missing_);
+ __missing_ = 0;
+ }
+ return *this;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __iterator& operator-=(difference_type __x)
+ requires random_access_range<_Base>
+ {
+ return *this += -__x;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr bool operator==(const __iterator& __x, const __iterator& __y) {
+ return __x.__current_ == __y.__current_;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr bool operator==(const __iterator& __x, default_sentinel_t) {
+ return __x.__current_ == __x.__end_;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr bool operator<(const __iterator& __x, const __iterator& __y)
+ requires random_access_range<_Base>
...
[truncated]
|
shyeyian
force-pushed
the
ranges_chunk
branch
3 times, most recently
from
December 9, 2025 00:39
7dd3f69 to
20d6a32
Compare
frederick-vs-ja
changed the title
std::views::chunkstd::views::chunk
shyeyian
force-pushed
the
ranges_chunk
branch
from
December 9, 2025 01:15
20d6a32 to
14d5e75
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
shyeyian
force-pushed
the
ranges_chunk
branch
from
December 9, 2025 01:23
14d5e75 to
25afd3b
Compare
frederick-vs-ja
left a comment
frederick-vs-ja
left a comment
Contributor
There was a problem hiding this comment.
Thanks! I've merely made quick style checks as for now.
H-G-Hristov
reviewed
Dec 9, 2025
H-G-Hristov
left a comment
H-G-Hristov
left a comment
Contributor
There was a problem hiding this comment.
Thank you for working on this. This is just a quick review.
Please follow the guidelines:
https://libcxx.llvm.org/Contributing.html#pre-commit-check-list
https://libcxx.llvm.org/CodingGuidelines.html
Make sure to test noexcept, such as: static_assert(noexcept(it.end())); and for constness if applicable.
Contributor
Forgot about that: The top comment is the commit message. Please put just what is relevant and what you want as a commit message in there. |
shyeyian
force-pushed
the
ranges_chunk
branch
from
December 9, 2025 04:56
a3bd864 to
99d5384
Compare
H-G-Hristov
reviewed
Dec 9, 2025
H-G-Hristov
reviewed
Dec 9, 2025
Contributor
Author
@frederick-vs-ja Thank you very much for your code review! I've applied all your suggestions in the recent new-pushed-commits. This is my first time contributing to libc++, if there is still anything wrong on my code, please point it out! :)
@H-G-Hristov Thank you for you code review too!
The CI seems to be all successful on both GCC and Clang, but failed on clang-cl. It will take me some time to debug the clang-cl CI failure (as I only have Linux and MacOS computers on my hand, and every time waiting for the online CI to produce a clang-cl error log, it needs more than 2 hours). Welcome to continue reviewing my changes! I'll appreciate any further code reviews. Thanks! |
Contributor
Maybe this is the issue: You have XFAILed the test but it looks like it is passing: Unexpectedly Passed: 1 (0.01%) |
shyeyian
force-pushed
the
ranges_chunk
branch
from
December 11, 2025 14:32
93d9a49 to
f7a08d9
Compare
Contributor
Author
|
Hello @frederick-vs-ja @H-G-Hristov I've updated the commits to meet your requirements (and following the code guidelines). May I re-request for a further code review or ask for merging? Thank you! (The mingw CI failure appears to be unrelated to the changes in |
Contributor
|
I think errors about libunwind are completely unrelated to this PR. Let's wait for the fix and then update the branch. |
shyeyian
force-pushed
the
ranges_chunk
branch
2 times, most recently
from
December 15, 2025 03:57
5f72824 to
b60a8c0
Compare
shyeyian
requested review from
JDevlieghere,
aaronmondal,
aaupov,
keith,
maksfb,
rafaelauler and
rupprecht
as code owners
December 15, 2025 03:57
huixie90
requested changes
Jun 21, 2026
huixie90
left a comment
huixie90
left a comment
Member
There was a problem hiding this comment.
Thanks for the patch. I had a quick review on the source code and it mostly looks good. I put some comments on the source code. I have not reviewed the tests yet but I have put some high level comments on the test. Thanks a lot
| std::ranges::chunk_view<std::ranges::ref_view<std::vector<int>>> chunked = vector | std::views::chunk(3); | ||
| std::ranges::chunk_view<input_span<int>> input_chunked = input_span<int>(vector) | std::views::chunk(3); | ||
|
|
||
| // Test `friend constexpr bool opreator==(const outer_iterator& x, default_sentinel_t)` |
Member
There was a problem hiding this comment.
I have not look at the tests in details. But I think the coverage is bit insufficient. Could you please add tests for the cases
- All the constraints need to be tested (with static_assert and concepts)
- negative cases. (for example, for this test
==we also need to test when it does not equal to default sentinel) - empty ranges
- a range with only 1 element
- a range with more than 1 element
- fully divisible ranges e.g. chunk of 3 with 12 elements
- not fully divisible ranges chunk of 3 with 11 elements
- chunk of 1
- chunk of N > 1
- chunk of N > size of the range
- different types of underlying iterators , we have lots of iterator types in test_iterators.h and test_ranges.h, and usually we template the tests with different types of the iterators/sentinel types
These comments applies to all of the tests
| std::vector<int> vector = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12}; | ||
| std::ranges::chunk_view<std::ranges::ref_view<std::vector<int>>> chunked = vector | std::views::chunk(3); | ||
| std::ranges::chunk_view<input_span<int>> input_chunked = input_span<int>(vector) | std::views::chunk(3); | ||
|
|
Member
There was a problem hiding this comment.
for this particular test, I think we need to test the branch where
_parent->_remainder == 0;
| } | ||
|
|
||
| public: | ||
| using iterator_category = input_iterator_tag; |
Member
There was a problem hiding this comment.
please add tests for these typdef
| return *this; | ||
| } | ||
|
|
||
| _LIBCPP_HIDE_FROM_ABI constexpr __iterator operator--(int) { |
Member
There was a problem hiding this comment.
missing constraints. That probably means that the test that tests the constraints are also missing
| ranges::advance(__current_, __n_ * (__x - 1)); | ||
| __missing_ = ranges::advance(__current_, __n_, __end_); | ||
| } else if (__x < 0) { | ||
| ranges::advance(__current_, __n_ * __x + __missing_); |
Member
There was a problem hiding this comment.
i think the test for this branch is missing
Zingam
requested changes
Jun 26, 2026
Contributor
There was a problem hiding this comment.
I did a quick review of the [[nodiscard]] application and the related tests cases.
These are tracked here: #172124 but since this is a new PR we don't need to track it there but you can use it as a reference.
All comments bellow are also relevant to: #172948
Please update both PRs so I don't have to bother you with the same comments twice.
Also note that most [[nodiscard]] tests are implemented in a style to ease the review, which means all test cases were added in the test in the same order as they appear in the header and also if split in multiple files ordered by files.
FYI: Please also make sure to "complete" your PR and get the CI green before you ask for a review as some reviewers might not review it if the PR is draft or the CI has errors.
|
|
||
| _LIBCPP_HIDE_FROM_ABI constexpr void operator++(int) { ++*this; } | ||
|
|
||
| [[nodiscard]] _LIBCPP_HIDE_FROM_ABI friend constexpr bool |
Contributor
There was a problem hiding this comment.
Suggested change
| [[nodiscard]] _LIBCPP_HIDE_FROM_ABI friend constexpr bool | |
| _LIBCPP_HIDE_FROM_ABI friend constexpr bool |
https://libcxx.llvm.org/CodingGuidelines.html#apply-nodiscard-where-relevant
See:
[[nodiscard]]should not be applied to functions if Clang already diagnoses unused results, for example: - conversion functions - equality operators - relational operators
| _Parent* __parent, iterator_t<_Base> __current, range_difference_t<_Base> __missing = 0) | ||
| : __current_(__current), __end_(ranges::end(__parent->__base_)), __n_(__parent->__n_), __missing_(__missing) {} | ||
|
|
||
| [[nodiscard]] static consteval auto __get_iterator_concept() { |
Contributor
There was a problem hiding this comment.
Suggested change
| [[nodiscard]] static consteval auto __get_iterator_concept() { | |
| static consteval auto __get_iterator_concept() { |
We not apply [[nodiscard]] to internal functions usually.
| return *this += -__x; | ||
| } | ||
|
|
||
| [[nodiscard]] _LIBCPP_HIDE_FROM_ABI friend constexpr bool operator==(const __iterator& __x, const __iterator& __y) { |
Contributor
There was a problem hiding this comment.
Suggested change
| [[nodiscard]] _LIBCPP_HIDE_FROM_ABI friend constexpr bool operator==(const __iterator& __x, const __iterator& __y) { | |
| _LIBCPP_HIDE_FROM_ABI friend constexpr bool operator==(const __iterator& __x, const __iterator& __y) { |
Reminder: https://github.com/llvm/llvm-project/pull/171234/changes#r3479366689
Same as above and please fixe the operators bellow.
Comment on lines
+37
to
+39
| std::ranges::iter_move(view.begin()); | ||
| // expected-warning@+1 {{ignoring return value of function declared with 'nodiscard' attribute}} | ||
| std::ranges::iter_move(std::as_const(view).begin()); |
Contributor
There was a problem hiding this comment.
This tests std::ranges::iter_move and not the member iter_move.
Maybe not te best example but you want something like that:
Also there is no need to write to test cases if one is enough.
e.g.
Suggested change
| std::ranges::iter_move(view.begin()); | |
| // expected-warning@+1 {{ignoring return value of function declared with 'nodiscard' attribute}} | |
| std::ranges::iter_move(std::as_const(view).begin()); | |
| auto cIt = std::as_const(view).begin(); | |
| .... | |
| // expected-warning@+1 {{ignoring return value of function declared with 'nodiscard' attribute}} | |
| iter_move(cIt); |
Comment on lines
+70
to
+72
| range | std::views::chunk(3); | ||
| // expected-warning@+1 {{ignoring return value of function declared with 'nodiscard' attribute}} | ||
| std::views::reverse | std::views::chunk(3); |
Contributor
There was a problem hiding this comment.
Suggested change
| range | std::views::chunk(3); | |
| // expected-warning@+1 {{ignoring return value of function declared with 'nodiscard' attribute}} | |
| std::views::reverse | std::views::chunk(3); |
These test operator|. It is tested elsewhere. Please remove.
Comment on lines
+56
to
+63
| // expected-warning@+1 {{ignoring return value of function declared with 'nodiscard' attribute}} | ||
| (view.begin() == view.end()); | ||
| // expected-warning@+1 {{ignoring return value of function declared with 'nodiscard' attribute}} | ||
| (std::as_const(view).begin() == view.end()); | ||
| // expected-warning@+1 {{ignoring return value of function declared with 'nodiscard' attribute}} | ||
| (view.begin() == std::as_const(view).end()); | ||
| // expected-warning@+1 {{ignoring return value of function declared with 'nodiscard' attribute}} | ||
| (std::as_const(view).begin() == std::as_const(view).end()); |
Contributor
There was a problem hiding this comment.
Suggested change
| // expected-warning@+1 {{ignoring return value of function declared with 'nodiscard' attribute}} | |
| (view.begin() == view.end()); | |
| // expected-warning@+1 {{ignoring return value of function declared with 'nodiscard' attribute}} | |
| (std::as_const(view).begin() == view.end()); | |
| // expected-warning@+1 {{ignoring return value of function declared with 'nodiscard' attribute}} | |
| (view.begin() == std::as_const(view).end()); | |
| // expected-warning@+1 {{ignoring return value of function declared with 'nodiscard' attribute}} | |
| (std::as_const(view).begin() == std::as_const(view).end()); |
Please remove. In fact you have added many more [[nodiscard]]s that are untested but they need to be reverted as per: https://libcxx.llvm.org/CodingGuidelines.html#apply-nodiscard-where-relevant
…rator`, `__inner_iterator` and `__outer_iterator` at the weekend)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR implements
std::views::chunkin header<__ranges/chunk_view.h>.This is partial P2442R1 (as P2442R1 contains both
views::chunkandviews::slide).Closes #171258