[libcxx-commits] [PATCH] D121204: [libc++][ranges] Implement `lazy_split_view`.

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const marked 12 inline comments as done.
var-const added a comment.

@Mordante Thank you for the review, and sorry about the confusion with two patches. Addressed most of the comments in D107500 <https://reviews.llvm.org/D107500>.



================
Comment at: libcxx/include/__ranges/lazy_split_view.h:67
+public:
+  lazy_split_view()
+    requires default_initializable<_View> && default_initializable<_Pattern> = default;
----------------
Mordante wrote:
> This function should be marked `_LIBCPP_HIDE_FROM_ABI`, similar for most functions in this patch.
Done (I might have missed something, but at least most of them should be now marked).


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:224
+        return __tmp;
+      } else {
+        ++*this;
----------------
Mordante wrote:
> The LLVM coding-style prefers to omit curly-braces when there's only one statement in the branch. 
> (I noticed the same in other parts of this review.)
I strongly dislike the LLVM style on this point. I don't think a single closing brace adds any significant line noise (the opening brace, being on the same line as the `if` or `else` condition, is clearly negligible) but it makes visually parsing code easier and protects against potential bugs. Personally, I would only use `if` without braces if the body is short enough to be on the same line. I'd rather keep this as is unless you feel strongly about it.


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:260
+    using iterator_concept = typename __outer_iterator<_Const>::iterator_concept;
+    using value_type = range_value_t<_Base>;
+    using difference_type = range_difference_t<_Base>;
----------------
Mordante wrote:
> I miss the public `iterator_category`.
It's inherited from `__inner_iterator_category` which was mistakenly inherited privately. Fixed now.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp:10
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: libcpp-no-concepts
+// UNSUPPORTED: libcpp-has-no-incomplete-ranges
----------------
Mordante wrote:
> This is no longer required, same for the other headers. (It was when you uploaded the patch.)
Thanks, removed those.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.range.pass.cpp:26
+template <class V, class P, class R, class D>
+concept CanConstruct = requires(R&& r, D&& d) {
+  std::ranges::lazy_split_view<V, P>(std::forward<R>(r), std::move(d));
----------------
Mordante wrote:
> This looks odd `D&& d` which is moved instead of forwarded. When this is intended please add comment explaining why this is intended.
No longer applicable (the concept is replaced by using `is_constructible_from`).


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:75
+      std::ranges::lazy_split_view v(input, sep);
+      (void)v;
+    }
----------------
Mordante wrote:
> How do you feel about using ``[[maybe_unused]]` instead?
Done. FWIW, I don't like how verbose it is, but the fact that it can be applied to the same line is nice.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:340
+
+  // Empty separator.
+  {
----------------
Mordante wrote:
> Is '\0' a valid separator?
Great suggestion, added a test. I think any character is a valid separator.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:393
+    }
+  }
+
----------------
Mordante wrote:
> I think it would be nice to also test with `wchar_t`, `char(8|16|32)_t`.
Added a single test case for each of those character types, is that sufficient?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer/ctor.copy.pass.cpp:15
+
+int main(int, char**) {
+  return 0;
----------------
Mordante wrote:
> What is the purpose of these empty tests? When intended please add comment why it's intended.
Sorry, I had a comment about this in the main patch but not here -- basically, I uploaded the patch for initial review while the tests were not finished yet, these are just placeholders for unfinished tests. All the tests (that I'm aware of) are implemented now, PTAL.


================
Comment at: libcxx/test/support/test_iterators.h:519
 };
+#ifndef _LIBCPP_HAS_NO_CONCEPTS
+  static_assert(std::input_iterator<cpp20_input_iterator<int*>>);
----------------
Mordante wrote:
> This is no longer required. (It was when you uploaded the patch.)
Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121204