[libcxx-commits] [PATCH] D136268: [libc++][ranges] implement `std::views::elements_view`

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added a comment.

Sorry about the very slow review. I have reviewed the code changes but haven't looked at the tests yet. Looking pretty good so far!



================
Comment at: libcxx/include/__iterator/concepts/can_reference.h:14
+#include <__config>
+
+
----------------
Nit: extraneous blank line.


================
Comment at: libcxx/include/__iterator/concepts/input_or_output_iterator.h:16
+#include <__iterator/concepts/weakly_incrementable.h>
+
+
----------------
Nit: extraneous blank line.


================
Comment at: libcxx/include/__iterator/concepts/weakly_incrementable.h:18
+#include <__iterator/incrementable_traits.h>
+
+
----------------
Nit: unnecessary extra blank line.


================
Comment at: libcxx/include/__ranges/elements_view.h:81
+  {
+    return __iterator<false>(ranges::begin(__base_));
+  }
----------------
Optional: consider using `__iterator</*_Const=*/false>`. Personally, I find this trick of "annotating" the meaning of literals pretty helpful when reading code.


================
Comment at: libcxx/include/__ranges/elements_view.h:141
+struct __elements_view_iterator_category_base<_Base, _Np> {
+  static consteval auto __get_iterator_category() {
+    using _Result = decltype(std::get<_Np>(*std::declval<iterator_t<_Base>>()));
----------------
This doesn't need `_LIBCPP_HIDE_FROM_ABI` because it's `consteval`?


================
Comment at: libcxx/include/__ranges/elements_view.h:178
+    } else {
+      using _Ele = remove_cv_t<tuple_element_t<_Np, range_reference_t<_Base>>>;
+      return static_cast<_Ele>(std::get<_Np>(*__i));
----------------
Question: what does `_Ele` stand for? Abbreviated names like this usually end with a consonant.


================
Comment at: libcxx/include/__ranges/elements_view.h:183
+
+  static consteval auto __get_iterator_concept() {
+    if constexpr (random_access_range<_Base>) {
----------------
Optional: I wonder if a generic `__get_iterator_concept` function that allows setting a "maximum" (most constrained) tag would be useful. This function is mostly the same as the one in `ranges_iterator_concept.h`, except it can never return a `contiguous_iterator`. It might be overkill, though -- and in either case, it's just a thought, not something to address in this patch.


================
Comment at: libcxx/include/__ranges/elements_view.h:335
+  using _Base              = __maybe_const<_Const, _View>;
+  sentinel_t<_Base> __end_ = sentinel_t<_Base>();
+
----------------
Would this benefit from `no_unique_address`?


================
Comment at: libcxx/include/__ranges/elements_view.h:343
+  __get_current(const elements_view<_View, _Np>::__iterator<_AnyConst>& __iter) {
+    return (__iter.__current_);
+  }
----------------
Question: the parentheses are to make sure `decltype(auto)` deduces a reference, right?


================
Comment at: libcxx/include/__tuple/make_tuple_types.h:65
+  template <size_t _Index>
+  using __element_type = std::conditional_t<_Index == 0, _Iter, _Sent>;
+  template <class _Tp, class _ApplyFn = __apply_cv_t<_Tp>>
----------------
Question: can you explain the idea here?


================
Comment at: libcxx/include/__tuple/make_tuple_types.h:66
+  using __element_type = std::conditional_t<_Index == 0, _Iter, _Sent>;
+  template <class _Tp, class _ApplyFn = __apply_cv_t<_Tp>>
+  using __apply_quals = __tuple_types< typename _ApplyFn::template __apply<__element_type<_Idx>>... >;
----------------
Nit: can you separate these two aliases with a blank line?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136268/new/

https://reviews.llvm.org/D136268