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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:224
+        return __tmp;
+      } else {
+        ++*this;
----------------
var-const wrote:
> 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.
I initially also disliked it, but it has kind of grown on me. At least nowadays compilers can warn about the misleading indention and when using clang-format the indention is always right.

Since we already use both styles in libc++ I don't feel too strongly about it. But other reviewers might disagree.


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


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:393
+    }
+  }
+
----------------
var-const wrote:
> 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?
Yes that looks sufficient. I see a small issue, but will post that in D107500.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121204