[libcxx-commits] [PATCH] D157193: [libc++][ranges] P2116R9: Implements `views::enumerate`
Konstantin Varlamov via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Sep 8 21:19:45 PDT 2023
var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.
Thank you for the patch! All in all it looks pretty good already. I have only started going through tests but wanted to share the feedback I already have.
================
Comment at: libcxx/docs/Status/Cxx2cIssues.csv:12
+"`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","|Nothing To Do|","","|ranges|"
"`3915 <https://wg21.link/LWG3915>`__","Redundant paragraph about expression variations","Varna June 2023","","","|ranges|"
----------------
IMO this should be `Complete` as well (I think `Nothing To Do` is for things like semantic requirements that are not enforceable in code).
================
Comment at: libcxx/docs/Status/RangesViews.csv:38
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,No patch yet,In Progress
+C++23,`enumerate <https://wg21.link/P2164R9>`_,Hristo Hristov,No patch yet,✅
----------------
Nit: please add a link to the patch (replacing the `No patch yet` part).
================
Comment at: libcxx/include/__ranges/concepts.h:146
+
+ // [concept.object]
+
----------------
Do we need this annotation? Where is it coming from?
================
Comment at: libcxx/include/__ranges/concepts.h:149
+ template <class _Rp>
+ concept __range_with_movable_references =
+ ranges::input_range<_Rp> && std::move_constructible<ranges::range_reference_t<_Rp>> &&
----------------
Since it's only used in `enumerate_view`, I would move the concept to that header (that's what we do for other view-specific concepts).
================
Comment at: libcxx/include/__ranges/enumerate_view.h:48
+
+ // [range.enumerate.iterator], class template enumerate_view::iterator
+ template <bool _Const>
----------------
Nit: I don't think the `, class template enumerate_view::iterator` part adds much value, it just restates the declaration that immediately follows. I'd remove the whole comment, but if you'd prefer to keep it, then let's keep just the section name.
================
Comment at: libcxx/include/__ranges/enumerate_view.h:56
+
+public:
+ template <bool _AnyConst>
----------------
Rather than making this public, can we use friends?
================
Comment at: libcxx/include/__ranges/enumerate_view.h:152
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __iterator(iterator_t<_Base> __current, difference_type __pos)
+ : __current_(std::move(__current)), __pos_(std::move(__pos)) {}
+
----------------
Do we need to move `__pos` (IIUC, it's always an integral type)?
================
Comment at: libcxx/include/__ranges/enumerate_view.h:160
+ requires _Const && convertible_to<iterator_t<_View>, iterator_t<_Base>>
+ : __current_(std::move(__i.__current_)), __pos_(std::move(__i.__pos_)){};
+
----------------
Nit: extraneous semicolon.
================
Comment at: libcxx/include/__ranges/enumerate_view.h:304
+
+// clang-format off
+struct __fn : __range_adaptor_closure<__fn> {
----------------
Optional: rather than turning off `clang-format`, you could use comments to force the text to align (the upside is not having to manually do or maintain other aspects of formatting). E.g. `as_rvalue_view` does this:
```
template <class _Range>
_LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Range&& __range) const
noexcept(noexcept(/**/ as_rvalue_view(std::forward<_Range>(__range))))
-> decltype(/*--*/ as_rvalue_view(std::forward<_Range>(__range))) {
return /*-------------*/ as_rvalue_view(std::forward<_Range>(__range));
```
================
Comment at: libcxx/include/ranges:320
+ requires see below
+ class enumerate_view; // freestanding, since C++23
+
----------------
I think we need to remove the `freestanding` comments since we don't implement freestanding yet.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/adaptor.pass.cpp:33-35
+constexpr void
+compareViews(View v,
+ std::initializer_list<std::tuple<typename std::ranges::iterator_t<View>::difference_type, T>> list) {
----------------
Nit: formatting around here looks a little surprising.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/adaptor.pass.cpp:40
+ auto e2 = list.end();
+ for (; b1 != e1 && b2 != e2; ++b1, ++b2) {
+ assert(*b1 == *b2);
----------------
Can this just be a call to `ranges::equal`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/adaptor.pass.cpp:75
+ compareViews(result, {{0, 'b'}, {1, 'a'}, {2, 'b'}, {3, 'a'}, {4, 'z'}, {5, 'm'}, {6, 't'}});
+ }
+ // Test `adaptor | views::enumerate`
----------------
Nit: please add a blank line after this line.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/base.pass.cpp:30
+
+ std::ranges::enumerate_view<Range> const view{range};
+ std::same_as<Range> decltype(auto) result = view.base();
----------------
Nit: can we add a test case that uses a non-const view (without moving)?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/base.pass.cpp:35
+ assert(range.end() == result.end());
+ }
+ // Check the && overload
----------------
Nit: please add a blank line between different test cases. Applies throughout the patch.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/begin.pass.cpp:16
+// constexpr auto begin() requires (!simple-view<V>)
+// { return iterator<false>(ranges::begin(base_), 0); }
+// constexpr auto begin() const requires range-with-movable-references<const V>
----------------
Nit: this comment should contain just the function declaration, without the definition. Applies throughout.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/begin.pass.cpp:29
+
+ // Check the return type of `.begin()`
+ {
----------------
Can we check both the case of `iterator<false>` and of `iterator<true>`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/begin.pass.cpp:35
+ using Iterator = std::ranges::iterator_t<decltype(view)>;
+ ASSERT_SAME_TYPE(Iterator, decltype(view.begin()));
+ }
----------------
Nit: I think we've been moving away from using `ASSERT_SAME_TYPE` towards just doing a `static_assert` directly.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/begin.pass.cpp:54
+ }
+ // begin() over a N-element range
+ {
----------------
Ultranit: `s/a/an/`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/enable_borrowed_range.compile.pass.cpp:16
+// template<class View>
+// constexpr bool enable_borrowed_range<enumerate_view<View>> = // freestanding
+// enable_borrowed_range<View>;
----------------
Nit: please remove the `freestanding` bit (throughout).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/end.pass.cpp:43
+ using Iterator = std::ranges::iterator_t<decltype(view)>;
+ ASSERT_SAME_TYPE(Iterator, decltype(view.end()));
+ }
----------------
We also need to check the case where `end` returns a sentinel, and both the const and non-const cases.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/end.pass.cpp:45
+ }
+ // begin() over an empty range
+ {
----------------
`s/begin/end/`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/end.pass.cpp:50
+ std::ranges::enumerate_view view(range);
+ auto it = view.begin();
+ assert(base(it.base()) == buff);
----------------
Should be `.end()`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/types.h:18
+
+struct Range : std::ranges::view_base {
+ using Iterator = cpp20_input_iterator<int*>;
----------------
Does this need to be a view? If it does, then I think it should be named `View` and not just `Range`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/types.h:38
+
+static_assert(std::ranges::__range_with_movable_references<Range>);
+static_assert(std::ranges::range<Range> && std::ranges::view<Range>);
----------------
When accessing internal types, we need to use `_LIBCPP_STATIC_ASSERT`. That macro is a no-op when tests are run by other implementations (e.g. MSVC) -- our test suite is used by other implementations as well.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/types.h:41
+
+struct SizedRange : public Range {
+ std::size_t size_;
----------------
Can we `static_assert` that this class satisfies `sized_range`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157193/new/
https://reviews.llvm.org/D157193
More information about the libcxx-commits
mailing list