Note that this class was called just split_view in the original One
Ranges Proposal and was renamed to lazy_split_view by
P2210.
Details
- Reviewers
- None
- Group Reviewers
Restricted Project
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
I had a look at the code, however I feel I'm not the most qualified person to review this code. So I left comment, but I leave the approval to somebody more familiar with this part of ranges.
libcxx/include/__ranges/lazy_split_view.h | ||
---|---|---|
9 | Nit: we usually have a blank line before the include guard. | |
67 | This function should be marked _LIBCPP_HIDE_FROM_ABI, similar for most functions in this patch. | |
224 | The LLVM coding-style prefers to omit curly-braces when there's only one statement in the branch. | |
260 | I miss the public iterator_category. | |
310 | Please move the return on its own line, since that's the LLVM style. (I know this matches the Standard wording.) | |
libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp | ||
10 | This is no longer required, same for the other headers. (It was when you uploaded the patch.) | |
libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.range.pass.cpp | ||
26 | This looks odd D&& d which is moved instead of forwarded. When this is intended please add comment explaining why this is intended. | |
libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp | ||
75 | How do you feel about using `[[maybe_unused]] instead? | |
340 | Is '\0' a valid separator? | |
393 | I think it would be nice to also test with wchar_t, char(8|16|32)_t. | |
libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer/ctor.copy.pass.cpp | ||
15 | What is the purpose of these empty tests? When intended please add comment why it's intended. | |
libcxx/test/std/ranges/range.adaptors/range.lazy.split/types.h | ||
103 | This looks copy pasted, please replace TEST_CONSTEXPR.* with constexpr. | |
libcxx/test/support/test_iterators.h | ||
519 | This is no longer required. (It was when you uploaded the patch.) |
libcxx/include/__ranges/lazy_split_view.h | ||
---|---|---|
67 | Done (I might have missed something, but at least most of them should be now marked). | |
224 | 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. | |
260 | It's inherited from __inner_iterator_category which was mistakenly inherited privately. Fixed now. | |
libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp | ||
10 | Thanks, removed those. | |
libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.range.pass.cpp | ||
26 | No longer applicable (the concept is replaced by using is_constructible_from). | |
libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp | ||
75 | Done. FWIW, I don't like how verbose it is, but the fact that it can be applied to the same line is nice. | |
340 | Great suggestion, added a test. I think any character is a valid separator. | |
393 | Added a single test case for each of those character types, is that sufficient? | |
libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer/ctor.copy.pass.cpp | ||
15 | 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. | |
libcxx/test/support/test_iterators.h | ||
519 | Thanks! |
libcxx/include/__ranges/lazy_split_view.h | ||
---|---|---|
224 | 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. | |
libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp | ||
340 | I also assume every character is valid. But this is a nice corner case. | |
393 | Yes that looks sufficient. I see a small issue, but will post that in D107500. |
clang-format not found in user’s local PATH; not linting file.