This is an archive of the discontinued LLVM Phabricator instance.

[libc++][ranges] Implement `lazy_split_view`.
AbandonedPublic

Authored by var-const on Mar 8 2022, 2:46 AM.

Details

Reviewers
None
Group Reviewers
Restricted Project
Summary

Note that this class was called just split_view in the original One
Ranges Proposal and was renamed to lazy_split_view by
P2210.

Diff Detail

Event Timeline

var-const created this revision.Mar 8 2022, 2:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2022, 2:46 AM
Herald added a subscriber: mgorny. · View Herald Transcript
var-const requested review of this revision.Mar 8 2022, 2:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2022, 2:46 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

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.
(I noticed the same in other parts of this review.)

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.
(I assume it's not possible to use the original.)

libcxx/test/support/test_iterators.h
519

This is no longer required. (It was when you uploaded the patch.)

var-const abandoned this revision.Mar 16 2022, 11:43 AM
var-const marked 12 inline comments as done.Mar 16 2022, 7:12 PM

@Mordante Thank you for the review, and sorry about the confusion with two patches. Addressed most of the comments in D107500.

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!

Mordante added inline comments.Mar 19 2022, 10:09 AM
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.