[libc++][ranges] P2116R9: Implements views::enumerate
#73617
+3,003
−7
Conversation
This file contains 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
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Hristo Hristov (H-G-Hristov) ChangesPatch is 111.99 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/73617.diff 41 Files Affected:
diff --git a/libcxx/docs/FeatureTestMacroTable.rst b/libcxx/docs/FeatureTestMacroTable.rst
index 3b2dff3108b0f60..cd1190e25bfe176 100644
--- a/libcxx/docs/FeatureTestMacroTable.rst
+++ b/libcxx/docs/FeatureTestMacroTable.rst
@@ -354,6 +354,8 @@ Status
--------------------------------------------------- -----------------
``__cpp_lib_ranges_chunk_by`` ``202202L``
--------------------------------------------------- -----------------
+ ``__cpp_lib_ranges_enumerate`` ``202302L``
+ --------------------------------------------------- -----------------
``__cpp_lib_ranges_iota`` *unimplemented*
--------------------------------------------------- -----------------
``__cpp_lib_ranges_join_with`` *unimplemented*
diff --git a/libcxx/docs/Status/Cxx23Papers.csv b/libcxx/docs/Status/Cxx23Papers.csv
index 5cc9e488297b9f7..5083a26e4d21b97 100644
--- a/libcxx/docs/Status/Cxx23Papers.csv
+++ b/libcxx/docs/Status/Cxx23Papers.csv
@@ -105,7 +105,7 @@
"","","","","","",""
"`P0290R4 <https://wg21.link/P0290R4>`__","LWG", "``apply()`` for ``synchronized_value<T>``","February 2023","","","|concurrency TS|"
"`P2770R0 <https://wg21.link/P2770R0>`__","LWG", "Stashing stashing ``iterators`` for proper flattening","February 2023","|In Progress|","","|ranges|"
-"`P2164R9 <https://wg21.link/P2164R9>`__","LWG", "``views::enumerate``","February 2023","","","|ranges|"
+"`P2164R9 <https://wg21.link/P2164R9>`__","LWG", "``views::enumerate``","February 2023","|Complete|","18.0","|ranges|"
"`P2711R1 <https://wg21.link/P2711R1>`__","LWG", "Making multi-param constructors of ``views`` ``explicit``","February 2023","|In Progress| [#note-P2711R1]_","","|ranges|"
"`P2609R3 <https://wg21.link/P2609R3>`__","LWG", "Relaxing Ranges Just A Smidge","February 2023","","","|ranges|"
"`P2713R1 <https://wg21.link/P2713R1>`__","LWG", "Escaping improvements in ``std::format``","February 2023","","","|format|"
diff --git a/libcxx/docs/Status/Cxx2cIssues.csv b/libcxx/docs/Status/Cxx2cIssues.csv
index fe0f13f6e8cb2c2..d12dc23cb4a8ca3 100644
--- a/libcxx/docs/Status/Cxx2cIssues.csv
+++ b/libcxx/docs/Status/Cxx2cIssues.csv
@@ -8,8 +8,8 @@
"`3903 <https://wg21.link/LWG3903>`__","span destructor is redundantly noexcept","Varna June 2023","|Complete|","7.0",""
"`3904 <https://wg21.link/LWG3904>`__","``lazy_split_view::outer-iterator``'s const-converting constructor isn't setting ``trailing_empty_``","Varna June 2023","","","|ranges|"
"`3905 <https://wg21.link/LWG3905>`__","Type of ``std::fexcept_t``","Varna June 2023","|Complete|","3.4",""
-"`3912 <https://wg21.link/LWG3912>`__","``enumerate_view::iterator::operator-`` should be ``noexcept``","Varna June 2023","","","|ranges|"
-"`3914 <https://wg21.link/LWG3914>`__","Inconsistent template-head of ``ranges::enumerate_view``","Varna June 2023","","","|ranges|"
+"`3912 <https://wg21.link/LWG3912>`__","``enumerate_view::iterator::operator-`` should be ``noexcept``","Varna June 2023","|Complete|","18.0","|ranges|"
+"`3914 <https://wg21.link/LWG3914>`__","Inconsistent template-head of ``ranges::enumerate_view``","Varna June 2023","|Complete|","18.0","|ranges|"
"`3915 <https://wg21.link/LWG3915>`__","Redundant paragraph about expression variations","Varna June 2023","","","|ranges|"
"`3925 <https://wg21.link/LWG3925>`__","Concept ``formattable``'s definition is incorrect","Varna June 2023","|Complete|","17.0","|format|"
"`3927 <https://wg21.link/LWG3927>`__","Unclear preconditions for ``operator[]`` for sequence containers","Varna June 2023","|Nothing To Do|","",""
diff --git a/libcxx/docs/Status/RangesViews.csv b/libcxx/docs/Status/RangesViews.csv
index f141656eb131a26..581596724878b25 100644
--- a/libcxx/docs/Status/RangesViews.csv
+++ b/libcxx/docs/Status/RangesViews.csv
@@ -35,4 +35,4 @@ C++23,`chunk_by <https://wg21.link/P2443R1>`_,Jakub Mazurkiewicz,`D144767 <https
C++23,`as_const <https://wg21.link/P2278R4>`_,Unassigned,No patch yet,Not started
C++23,`as_rvalue <https://wg21.link/P2446R2>`_,Nikolas Klauser,`D137637 <https://llvm.org/D137637>`_,✅
C++23,`stride <https://wg21.link/P1899R3>`_,Hristo Hristov and Will Hawkins,`D156924 <https://llvm.org/D156924>`_,In Progress
-C++23,`enumerate <https://wg21.link/P2164R9>`_,Hristo Hristov,`D157193 <https://reviews.llvm.org/D157193>`_,In Progress
+C++23,`enumerate <https://wg21.link/P2164R9>`_,Hristo Hristov,`D157193 <https://reviews.llvm.org/D157193>`_,✅
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index b0bc3718576f66b..eca80630bb1c1ef 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -622,6 +622,7 @@ set(files
__ranges/empty_view.h
__ranges/enable_borrowed_range.h
__ranges/enable_view.h
+ __ranges/enumerate_view.h
__ranges/filter_view.h
__ranges/from_range.h
__ranges/iota_view.h
diff --git a/libcxx/include/__ranges/enumerate_view.h b/libcxx/include/__ranges/enumerate_view.h
new file mode 100644
index 000000000000000..77b29837ba54603
--- /dev/null
+++ b/libcxx/include/__ranges/enumerate_view.h
@@ -0,0 +1,333 @@
+// -*- 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_ENUMERATE_VIEW_H
+#define _LIBCPP___RANGES_ENUMERATE_VIEW_H
+
+#include <__concepts/convertible_to.h>
+#include <__config>
+#include <__iterator/concepts.h>
+#include <__iterator/distance.h>
+#include <__iterator/iter_move.h>
+#include <__iterator/iterator_traits.h>
+#include <__ranges/access.h>
+#include <__ranges/all.h>
+#include <__ranges/concepts.h>
+#include <__ranges/enable_borrowed_range.h>
+#include <__ranges/range_adaptor.h>
+#include <__ranges/size.h>
+#include <__ranges/view_interface.h>
+#include <__type_traits/maybe_const.h>
+#include <__utility/forward.h>
+#include <__utility/move.h>
+#include <tuple>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+# pragma GCC system_header
+#endif
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+#if _LIBCPP_STD_VER >= 23
+
+namespace ranges {
+
+// [concept.object]
+
+template <class _Rp>
+concept __range_with_movable_references =
+ ranges::input_range<_Rp> && std::move_constructible<ranges::range_reference_t<_Rp>> &&
+ std::move_constructible<ranges::range_rvalue_reference_t<_Rp>>;
+
+// [range.enumerate.view]
+
+template <view _View>
+ requires __range_with_movable_references<_View>
+class enumerate_view : public view_interface<enumerate_view<_View>> {
+ _View __base_ = _View();
+
+ // [range.enumerate.iterator]
+ template <bool _Const>
+ class __iterator;
+
+ // [range.enumerate.sentinel]
+ template <bool _Const>
+ class __sentinel;
+
+ template <bool _AnyConst>
+ _LIBCPP_HIDE_FROM_ABI static constexpr decltype(auto) __get_current(const __iterator<_AnyConst>& __iter) {
+ return (__iter.__current_);
+ }
+
+public:
+ _LIBCPP_HIDE_FROM_ABI constexpr enumerate_view()
+ requires(default_initializable<_View>)
+ = default;
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit enumerate_view(_View __base) : __base_(std::move(__base)){};
+
+ _LIBCPP_HIDE_FROM_ABI constexpr auto begin()
+ requires(!__simple_view<_View>)
+ {
+ return __iterator<false>(ranges::begin(__base_), 0);
+ }
+ _LIBCPP_HIDE_FROM_ABI constexpr auto begin() const
+ requires __range_with_movable_references<const _View>
+ {
+ return __iterator<true>(ranges::begin(__base_), 0);
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr auto end()
+ requires(!__simple_view<_View>)
+ {
+ if constexpr (common_range<_View> && sized_range<_View>)
+ return __iterator<false>(ranges::end(__base_), ranges::distance(__base_));
+ else
+ return __sentinel<false>(ranges::end(__base_));
+ }
+ _LIBCPP_HIDE_FROM_ABI constexpr auto end() const
+ requires __range_with_movable_references<const _View>
+ {
+ if constexpr (common_range<const _View> && sized_range<const _View>)
+ return __iterator<true>(ranges::end(__base_), ranges::distance(__base_));
+ else
+ return __sentinel<true>(ranges::end(__base_));
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr auto size()
+ requires sized_range<_View>
+ {
+ return ranges::size(__base_);
+ }
+ _LIBCPP_HIDE_FROM_ABI constexpr auto size() const
+ requires sized_range<const _View>
+ {
+ return ranges::size(__base_);
+ }
+
+ _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_); }
+};
+
+template <class _Range>
+enumerate_view(_Range&&) -> enumerate_view<views::all_t<_Range>>;
+
+// [range.enumerate.iterator]
+
+template <view _View>
+ requires __range_with_movable_references<_View>
+template <bool _Const>
+class enumerate_view<_View>::__iterator {
+ using _Base = __maybe_const<_Const, _View>;
+
+ 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 if constexpr (forward_range<_Base>) {
+ return forward_iterator_tag{};
+ } else {
+ return input_iterator_tag{};
+ }
+ }
+
+ friend class enumerate_view<_View>;
+
+public:
+ using iterator_category = input_iterator_tag;
+ using iterator_concept = decltype(__get_iterator_concept());
+ using difference_type = range_difference_t<_Base>;
+ using value_type = tuple<difference_type, range_value_t<_Base>>;
+
+private:
+ using __reference_type = tuple<difference_type, range_reference_t<_Base>>;
+ iterator_t<_Base> __current_ = iterator_t<_Base>();
+ difference_type __pos_ = 0;
+
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __iterator(iterator_t<_Base> __current, difference_type __pos)
+ : __current_(std::move(__current)), __pos_(__pos) {}
+
+public:
+ _LIBCPP_HIDE_FROM_ABI __iterator()
+ requires(default_initializable<iterator_t<_Base>>)
+ = default;
+ _LIBCPP_HIDE_FROM_ABI constexpr __iterator(__iterator<!_Const> __i)
+ requires _Const && convertible_to<iterator_t<_View>, iterator_t<_Base>>
+ : __current_(std::move(__i.__current_)), __pos_(__i.__pos_) {}
+
+ _LIBCPP_HIDE_FROM_ABI constexpr const iterator_t<_Base>& base() const& noexcept { return __current_; }
+ _LIBCPP_HIDE_FROM_ABI constexpr iterator_t<_Base> base() && { return std::move(__current_); }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr difference_type index() const noexcept { return __pos_; }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr auto operator*() const { return __reference_type(__pos_, *__current_); }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __iterator& operator++() {
+ ++__current_;
+ ++__pos_;
+ return *this;
+ }
+ _LIBCPP_HIDE_FROM_ABI constexpr void operator++(int) { return ++*this; }
+ _LIBCPP_HIDE_FROM_ABI constexpr __iterator operator++(int)
+ requires forward_range<_Base>
+ {
+ auto __temp = *this;
+ ++*this;
+ return __temp;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __iterator& operator--()
+ requires bidirectional_range<_Base>
+ {
+ --__current_;
+ --__pos_;
+ return *this;
+ }
+ _LIBCPP_HIDE_FROM_ABI constexpr __iterator operator--(int)
+ requires bidirectional_range<_Base>
+ {
+ auto __temp = *this;
+ --*this;
+ return *__temp;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __iterator& operator+=(difference_type __n)
+ requires random_access_range<_Base>
+ {
+ __current_ += __n;
+ __pos_ += __n;
+ return *this;
+ }
+ _LIBCPP_HIDE_FROM_ABI constexpr __iterator& operator-=(difference_type __n)
+ requires random_access_range<_Base>
+ {
+ __current_ -= __n;
+ __pos_ -= __n;
+ return *this;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr auto operator[](difference_type __n) const
+ requires random_access_range<_Base>
+ {
+ return __reference_type(__pos_ + __n, __current_[__n]);
+ }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr bool operator==(const __iterator& __x, const __iterator& __y) noexcept {
+ return __x.__pos_ == __y.__pos_;
+ }
+ _LIBCPP_HIDE_FROM_ABI friend constexpr strong_ordering
+ operator<=>(const __iterator& __x, const __iterator& __y) noexcept {
+ return __x.__pos_ <=> __y.__pos_;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr __iterator operator+(const __iterator& __i, difference_type __n)
+ requires random_access_range<_Base>
+ {
+ auto __temp = __i;
+ __temp += __n;
+ return __temp;
+ }
+ _LIBCPP_HIDE_FROM_ABI friend constexpr __iterator operator+(difference_type __n, const __iterator& __i)
+ requires random_access_range<_Base>
+ {
+ return __i + __n;
+ }
+ _LIBCPP_HIDE_FROM_ABI friend constexpr __iterator operator-(const __iterator& __i, difference_type __n)
+ requires random_access_range<_Base>
+ {
+ auto __temp = __i;
+ __temp -= __n;
+ return __temp;
+ }
+ _LIBCPP_HIDE_FROM_ABI friend constexpr difference_type
+ operator-(const __iterator& __x, const __iterator& __y) noexcept {
+ return __x.__pos_ - __y.__pos_;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr auto iter_move(const __iterator& __i) noexcept(
+ noexcept(ranges::iter_move(__i.__current_)) && is_nothrow_move_constructible_v<range_rvalue_reference_t<_Base>>) {
+ return tuple<difference_type, range_rvalue_reference_t<_Base>>(__i.__pos_, ranges::iter_move(__i.__current_));
+ }
+};
+
+// [range.enumerate.sentinel]
+
+template <view _View>
+ requires __range_with_movable_references<_View>
+template <bool _Const>
+class enumerate_view<_View>::__sentinel {
+ using _Base = __maybe_const<_Const, _View>;
+ sentinel_t<_Base> __end_ = sentinel_t<_Base>();
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __sentinel(sentinel_t<_Base> __end) : __end_(std::move(__end)) {}
+
+ friend class enumerate_view<_View>;
+
+public:
+ _LIBCPP_HIDE_FROM_ABI __sentinel() = default;
+ _LIBCPP_HIDE_FROM_ABI constexpr __sentinel(__sentinel<!_Const> __other)
+ requires _Const && convertible_to<sentinel_t<_View>, sentinel_t<_Base>>
+ : __end_(std::move(__other.__end_)) {}
+
+ _LIBCPP_HIDE_FROM_ABI constexpr sentinel_t<_Base> base() const { return __end_; }
+
+ template <bool _OtherConst>
+ requires sentinel_for<sentinel_t<_Base>, iterator_t<__maybe_const<_OtherConst, _View>>>
+ _LIBCPP_HIDE_FROM_ABI friend constexpr bool operator==(const __iterator<_OtherConst>& __x, const __sentinel& __y) {
+ return __get_current(__x) == __y.__end_;
+ }
+
+ template <bool _OtherConst>
+ requires sized_sentinel_for<sentinel_t<_Base>, iterator_t<__maybe_const<_OtherConst, _View>>>
+ _LIBCPP_HIDE_FROM_ABI friend constexpr range_difference_t<__maybe_const<_OtherConst, _View>>
+ operator-(const __iterator<_OtherConst>& __x, const __sentinel& __y) {
+ return __get_current(__x) - __y.__end_;
+ }
+
+ template <bool _OtherConst>
+ requires sized_sentinel_for<sentinel_t<_Base>, iterator_t<__maybe_const<_OtherConst, _View>>>
+ _LIBCPP_HIDE_FROM_ABI friend constexpr range_difference_t<__maybe_const<_OtherConst, _View>>
+ operator-(const __sentinel& __x, const __iterator<_OtherConst>& __y) {
+ return __x.__end_ - __get_current(__y);
+ }
+};
+
+template <class _View>
+constexpr bool enable_borrowed_range<enumerate_view<_View>> = enable_borrowed_range<_View>;
+
+namespace views {
+namespace __enumerate {
+
+struct __fn : __range_adaptor_closure<__fn> {
+ template <class _Range>
+ [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Range&& __range) const
+ noexcept(noexcept(/**/ enumerate_view(std::forward<_Range>(__range))))
+ -> decltype(/*--*/ enumerate_view(std::forward<_Range>(__range))) {
+ return /*-------------*/ enumerate_view(std::forward<_Range>(__range));
+ }
+};
+
+} // namespace __enumerate
+
+inline namespace __cpo {
+
+inline constexpr auto enumerate = __enumerate::__fn{};
+
+} // namespace __cpo
+} // namespace views
+} // namespace ranges
+
+#endif // _LIBCPP_STD_VER >= 23
+
+_LIBCPP_END_NAMESPACE_STD
+
+#endif // _LIBCPP___RANGES_ENUMERATE_VIEW_H
diff --git a/libcxx/include/module.modulemap.in b/libcxx/include/module.modulemap.in
index 90381f4f7f720d4..c0949a28b278b5a 100644
--- a/libcxx/include/module.modulemap.in
+++ b/libcxx/include/module.modulemap.in
@@ -1663,6 +1663,7 @@ module std_private_ranges_empty [system] { header "__ranges
module std_private_ranges_empty_view [system] { header "__ranges/empty_view.h" }
module std_private_ranges_enable_borrowed_range [system] { header "__ranges/enable_borrowed_range.h" }
module std_private_ranges_enable_view [system] { header "__ranges/enable_view.h" }
+module std_private_ranges_enumerate_view [system] { header "__ranges/enumerate_view.h" }
module std_private_ranges_filter_view [system] {
header "__ranges/filter_view.h"
export std_private_ranges_range_adaptor
diff --git a/libcxx/include/ranges b/libcxx/include/ranges
index f71a92f8a660b06..56b3c82daa11811 100644
--- a/libcxx/include/ranges
+++ b/libcxx/include/ranges
@@ -314,6 +314,17 @@ namespace std::ranges {
namespace views { template<class T> inline constexpr unspecified istream = unspecified; }
+ // [range.enumerate], enumerate view
+ template<view View>
+ requires see below
+ class enumerate_view; // since C++23
+
+ template<class View>
+ constexpr bool enable_borrowed_range<enumerate_view<View>> = // since C++23
+ enable_borrowed_range<View>;
+
+ namespace views { inline constexpr unspecified enumerate = unspecified; } // since C++23
+
// [range.zip], zip view
template<input_range... Views>
requires (view<Views> && ...) && (sizeof...(Views) > 0)
@@ -393,6 +404,7 @@ namespace std {
#include <__ranges/empty_view.h>
#include <__ranges/enable_borrowed_range.h>
#include <__ranges/enable_view.h>
+#include <__ranges/enumerate_view.h>
#include <__ranges/filter_view.h>
#include <__ranges/from_range.h>
#include <__ranges/iota_view.h>
diff --git a/libcxx/include/version b/libcxx/include/version
index ef01af753298222..ed6c2aa9fcd42fa 100644
--- a/libcxx/include/version
+++ b/libcxx/include/version
@@ -170,6 +170,7 @@ __cpp_lib_ranges_as_const 202207L <ranges>
__cpp_lib_ranges_as_rvalue 202207L <ranges>
__cpp_lib_ranges_chunk 202202L <ranges>
__cpp_lib_ranges_chunk_by 202202L <ranges>
+__cpp_lib_ranges_enumerate 202302L <ranges>
__cpp_lib_ranges_iota 202202L <numeric>
__cpp_lib_ranges_join_with 202202L <ranges>
__cpp_lib_ranges_repeat 202207L <ranges>
@@ -461,6 +462,7 @@ __cpp_lib_within_lifetime 202306L <type_traits>
# define __cpp_lib_ranges_as_rvalue 202207L
// # define __cpp_lib_ranges_chunk 202202L
# define __cpp_lib_ranges_chunk_by 202202L
+# define __cpp_lib_ranges_enumerate 202302L
// # define __cpp_lib_ranges_iota 202202L
// # define __cpp_lib_ranges_join_with 202202L
# define __cpp_lib_ranges_repeat 202207L
diff --git a/libcxx/modules/std/ranges.inc b/libcxx/modules/std/ranges.inc
index a883103d812588b..5b97f02c56a4957 100644
--- a/libcxx/modules/std/ranges.inc
+++ b/libcxx/modules/std/ranges.inc
@@ -320,9 +320,8 @@ export namespace std {
namespace views {
using std::ranges::views::chunk_by;
}
-#endif // _LIBCPP_STD_VER >= 23
-#if 0
+# if 0
// [range.stride], stride view
using std::ranges::stride_view;
@@ -335,7 +334,15 @@ export namespace std {
namespace views {
using std::ranges::views::cartesian_product;
}
-#endif
+# endif
+
+ // [range.enumerate]
+ using std::ranges::enumerate_view;
+
+ namespace views {
+ using std::ranges::views::enumerate;
+ }
+#end...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
var-const
approved these changes
Dec 6, 2023
There was a problem hiding this comment.
Thanks a lot for addressing most of the feedback! I don't think there's anything major left, so hopefully it would be ready to merge soon. I copied the unaddressed comments from the original review -- please let me know if you have any questions re. any of those (feel free to reach out on Discord).
| test<contiguous_iterator<int*>>(); | ||
| test<int*>(); | ||
|
|
||
| test<cpp17_input_iterator<int const*>, int const>(); |
There was a problem hiding this comment.
Question: why is this int const* (i.e., a const pointer) and not const int* (a pointer to const)?
There was a problem hiding this comment.
Those are both pointer-to-const. int* const is a const pointer.
libcxx/test/std/ranges/range.adaptors/range.enumerate/begin.pass.cpp
Outdated
Show resolved
Hide resolved
|
|
||
| // class enumerate_view::sentinel | ||
|
|
||
| // template<bool OtherConst> |
There was a problem hiding this comment.
Copying an old comment -- is it possible to also check the OtherConst case?
| // class enumerate_view | ||
|
|
||
| // constexpr auto size() requires sized_range<V>; | ||
| // constexpr auto size() const requires sized_range<const V>; |
There was a problem hiding this comment.
Copying an old comment -- can we check with a (pathological) type that is sized if it's non-const but not sized if it's const?
libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/equal.pass.cpp
Show resolved
Hide resolved
libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/types.compile.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/types.compile.pass.cpp
Show resolved
Hide resolved
var-const
requested changes
Dec 6, 2023
cjdb
requested changes
Jan 2, 2024
There was a problem hiding this comment.
Thanks for working on this! There's a fair bit that I've provided comments for, but I think you're off to a great start, and I would like to see this merged in January, if at all possible.
Some comments are short and repetitive: those are usually coupled with a starter comment that explains my perspective, and then I just flag the others as I see them in a (hopefully) non-intrusive way.
libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/types.compile.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/types.compile.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/minus.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/minus.pass.cpp
Outdated
Show resolved
Hide resolved
Comment on lines
+33
to
+79
| template <bool Const> | ||
| struct Sentinel { | ||
| constexpr bool operator==(const Iterator<Const>& i) const { return i.it_ == end_; } | ||
|
|
||
| std::tuple<std::ptrdiff_t, int>* end_; | ||
| }; | ||
|
|
||
| template <bool Const> | ||
| struct CrossComparableSentinel { | ||
| template <bool C> | ||
| constexpr bool operator==(const Iterator<C>& i) const { | ||
| return i.it_ == end_; | ||
| } | ||
|
|
||
| std::tuple<std::ptrdiff_t, int>* end_; | ||
| }; | ||
|
|
||
| template <bool Const> | ||
| struct SizedSentinel { | ||
| constexpr bool operator==(const Iterator<Const>& i) const { return i.it_ == end_; } | ||
|
|
||
| friend constexpr auto operator-(const SizedSentinel& st, const Iterator<Const>& it) { return st.end_ - it.it_; } | ||
|
|
||
| friend constexpr auto operator-(const Iterator<Const>& it, const SizedSentinel& st) { return it.it_ - st.end_; } | ||
|
|
||
| int* end_; | ||
| }; | ||
|
|
||
| template <bool Const> | ||
| struct CrossSizedSentinel { | ||
| template <bool C> | ||
| constexpr bool operator==(const Iterator<C>& i) const { | ||
| return i.it_ == end_; | ||
| } | ||
|
|
||
| template <bool C> | ||
| friend constexpr auto operator-(const CrossSizedSentinel& st, const Iterator<C>& it) { | ||
| return st.end_ - it.it_; | ||
| } | ||
|
|
||
| template <bool C> | ||
| friend constexpr auto operator-(const Iterator<C>& it, const CrossSizedSentinel& st) { | ||
| return it.it_ - st.end_; | ||
| } | ||
|
|
||
| int* end_; | ||
| }; |
There was a problem hiding this comment.
How do these differ from sentinel_wrapper and sized_sentinel in libcxx/test/support/test_iterators.h?
philnik777
reviewed
Jan 4, 2024
Thank you very much for the review! Sorry about the commented out code I broke some tests and commented them out. I try to keep the CI green. I also switch between two accounts. I'll make sure to clean up all artifacts for the final round. |
|
What's the status of this PR? |
I'm sorry for the delay. I still hope to get it done in time for LLVM 19 but I can't promise. I won't mind if somebody decides to give a helping hand to finish the PR faster. |
EricWF
reviewed
Jun 3, 2024
| #include "test_iterators.h" | ||
| #include "test_macros.h" | ||
|
|
||
| // Types |
There was a problem hiding this comment.
Are there not already archetypes like these in test/support?
I would really rather not duplicate them if-so.
Additionally, I don't love the relative include of such a header with such a non-descript name.
If these types aren't reusable, could you explain why? And if they are, could we put them in test/support and give the header a better name.
|
|
||
| constexpr explicit RangeView(int* b, int* e) : begin_(b), end_(e) {} | ||
| constexpr RangeView(RangeView const& other) : begin_(other.begin_), end_(other.end_), wasCopyInitialized(true) {} | ||
| constexpr RangeView(RangeView&& other) : begin_(other.begin_), end_(other.end_), wasMoveInitialized(true) {} |
There was a problem hiding this comment.
This should set other.begin_ and friends to nullptr or some other sentinal value.
I also worry about test types that track "was moved" and "was copied", because often times the last copy or move construction isn't the one you care about. (That said, there are types like this everywhere in the code base).
| int* end_; | ||
| }; | ||
|
|
||
| static_assert(std::ranges::range<NotAViewRange>); |
There was a problem hiding this comment.
I like the assertions on the test types.
|
|
||
| #include "types.h" | ||
|
|
||
| // Iterators & Sentinels |
There was a problem hiding this comment.
Why not put this in "types.h"?
The more headers and types we have defined outside of the test file,
the harder it gets to inspect the test for correction.
| // Iterators & Sentinels | ||
|
|
||
| template <bool Const> | ||
| struct Iterator { |
There was a problem hiding this comment.
Could we name these types better so the reader of the test can understand what they do and why the exist from the name alone?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Migrating review from: https://reviews.llvm.org/D157193 (some review comments not yet addressed)
Implements P2116R9 views::enumerate
https://wg21.link/P2164R9
https://wg21.link/range.enumerate / https://eel.is/c++draft/range.enumerate