[libcxx-commits] [PATCH] D107500: [libc++][ranges] Implement `lazy_split_view`.
Konstantin Varlamov via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Mar 15 19:50:41 PDT 2022
var-const added inline comments.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:152
+ using _MaybeCurrent = _If<forward_range<_View>, iterator_t<_Base>, __empty_cache>;
+ [[no_unique_address]] _MaybeCurrent __current_ = _MaybeCurrent();
+ bool __trailing_empty_ = false;
----------------
philnik wrote:
> You have to use `_LIBCPP_NO_UNIQUE_ADDRESS`. I think there are other uses of raw `[[no_unique_address]]`.
Thanks!
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:192
+ _LIBCPP_HIDE_FROM_ABI
+ constexpr default_sentinel_t end() const { return default_sentinel; }
+ };
----------------
philnik wrote:
> This should be `noexcept`. Please add a regression test.
Thanks for catching this! Added a test.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:226-228
+ ++__current();
+
+ } else if constexpr (__tiny_range<_Pattern>) {
----------------
philnik wrote:
>
I'd rather keep as is, otherwise the two branches merge into a monolithic block of code (and this seems like the most logical place to split).
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:271
+
+ friend constexpr bool operator==(const __outer_iterator& __x, default_sentinel_t) {
+ return __x.__current() == ranges::end(__x.__parent_->__base_) && !__x.__trailing_empty_;
----------------
philnik wrote:
> You missed a `_LIBCPP_HIDE_FROM_ABI` here.
Thanks!
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:355-356
+ const auto& __cur = __x.__i_.__current();
+ if (__cur == __end) return true;
+ if (__pcur == __pend) return __x.__incremented_;
+
----------------
philnik wrote:
> Ditto below
This formatting is copied from the Standard. I'm slightly in favor of keeping it consistent with the Standard (or otherwise I'd also add braces). Let me know if you feel strongly about this.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:82
+ _LIBCPP_HIDE_FROM_ABI
+ lazy_split_view()
+ requires default_initializable<_View> && default_initializable<_Pattern> = default;
----------------
philnik wrote:
> Could you add `constexpr`, just for symmetry (and it's marked as `constexpr` in the standard)?
Hmm, I don't think it's marked `constexpr` here: https://eel.is/c++draft/range.lazy.split.view Am I missing something?
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:89-95
+ template <input_range _Range>
+ requires constructible_from<_View, views::all_t<_Range>> &&
+ constructible_from<_Pattern, single_view<range_value_t<_Range>>>
+ _LIBCPP_HIDE_FROM_ABI
+ constexpr lazy_split_view(_Range&& __r, range_value_t<_Range> __e)
+ : __base_(views::all(std::forward<_Range>(__r)))
+ , __pattern_(ranges::single_view(std::move(__e))) {}
----------------
philnik wrote:
> The `views::single` change should probably be the same, but this is closer to the standards wording. If we don't define `views::single` yet ignore it.
Thanks for catching that! `views::single` is indeed not implemented yet -- added a TODO.
As for the constructor initialization list, we have a lot of precedent for this formatting (with leading commas), so I'd rather keep as is.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:40
+
+#if !defined(_LIBCPP_HAS_NO_RANGES)
+
----------------
Quuxplusone wrote:
> `#if !defined(_LIBCPP_HAS_NO_CONCEPTS) && !defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES)`
> (and likewise on the `#endif` of course)
>
I //think// that now that libc++ no longer supports older compilers, the `_LIBCPP_HAS_NO_CONCEPTS` is unnecessary; but it's a C++20 feature, so it seems like the guard should be `#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_RANGES)`.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:118
+
+ template <bool _Const> struct __outer_iterator : __outer_iterator_category<__maybe_const<_Const, _View>> {
+ private:
----------------
Quuxplusone wrote:
> var-const wrote:
> > The requirement is that `iterator_category` is only present if `_View` is a `forward_range`. I'm concerned that using inheritance here has ABI implications -- is there a better way to achieve this?
> Good thought! The thing to watch out for here is basically when the layout might lead to two "adjacent" empty bases of the same type. See
> https://quuxplusone.github.io/blog/2021/05/07/std-iterator-as-a-base-class/
> for a real-world libc++ example.
>
> In this case, I believe there's no problem, because the first data member of `__outer_iterator<B>` is definitely of type `_Parent*`. If you swapped the declaration order of `__parent_` and `__current_`, then we'd have a problem, because the type of `__current_` is some-user-controlled-iterator-type that might (might it?) inherit from `__outer_iterator_category<_View>`. So, don't swap those two data members, and you'll be fine.
>
> The inheritance thing is the same thing I did with `__move_iter_category_base` in https://reviews.llvm.org/D117656#change-18S54d7VLfJo and it's the best approach AFAIK. For consistency, how would you feel about renaming `__outer_iterator_category` to `__outer_iter[ator]_category_base`?
I'd rather avoid inconsistency between `__outer_iterator` and `__outer_iter_category` -- those 4 characters are not worth it, in my opinion.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:133
+ constexpr auto& __current() {
+ if constexpr (forward_range<_Base>) {
+ return __current_;
----------------
Quuxplusone wrote:
> This seems to match the wording in http://eel.is/c++draft/range.lazy.split.outer#1 , but it worries me that we're //storing// an iterator in `__outer_iterator<true>::__current_` iff `forward_range<_View>` but trying to //access// it iff `forward_range<const _View>`. What happens if I give this code a `_View` that is a range but not a const-iterable range, like say it's a `filter_view`; worse, what happens if I give this code a `_View` that isn't a range but its const version is? I haven't done my homework so I won't tag tcanens here yet ;) but if you dig into it and come out as confused as I feel right now, he's the one I'd recommend asking for clarification.
This is a very good catch. I think that the current implementation was in fact not conforming:
A.
```
iterator_t<Base> current_ = iterator_t<Base>(); // exposition only, present only
// if V models forward_range
```
That part was correct:
```
using _MaybeCurrent = _If<forward_range<_View>, iterator_t<_Base>, __empty_cache>;
```
In both cases, the check is for `forward_range<View>`, where `View` cannot be const-qualified.
B.
> //current// is equivalent to `current_` if `V` models `forward_range`...
That was incorrect -- in the `__current()` helper function, the `forward_range` check should operate on `_View`, not `_Base`. This is fixed now.
There are, however, several cases where `forward_range<Base>` and `forward_range<V>` seem to be used interchangeably, like in the constructor:
```
constexpr outer-iterator(Parent& parent, iterator_t<Base> current)
requires forward_range<Base>;
```
> Effects: Initializes `parent_` with `addressof(parent)` and `current_` with `std::move(current)`.
However, there still seems to be no way for this to create an actual problem (or at least I cannot come up with it). The difference between `_View` and `_Base` only arises if `_Const == true`. Whether the `__outer_iterator` is `_Const` is determined by the implementation of `begin()` and `end()` in the `lazy_split_view`. It looks like there are only two cases which result in `__outer_iterator<true>`:
1. `forward_range<_View> && forward_range<const _View>`'
2. `forward_range<_View> && __simple_view<_View>`
In the first case, there clearly can be no issue. For the second case, I don't //think// it's possible to come up with a case where `forward_range<_View> && __simple_view<_View>` but `!forward_range<const _View>`. `forward_range` essentially requires that it is a `range` and the iterator is both an `input_iterator` and a `forward_iterator` (where of course the latter subsumes the former). `__simple_view` requires that `range<const T>` -- which means that the only option for a view to not satisfy `forward_view<const V>` is for the iterator to be either an input iterator or not even an input iterator. However, the other requirement of `__simple_view` is that the iterator type is the same between `T` and `const T`. This seems to mean that:
- `V` has to satisfy `forward_range`;
- the only way for `const V` to not satisfy `forward_range` is to have a non-forward iterator;
- but `V` and `const V` are required to have the same iterator type.
Thus it seems impossible to come up with a case where `_View` and `_Base` in `__outer_iterator` have different const-qualification //and// `forward_range<_View> && !forward_range<const _View>`. It is definitely complicated, though.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:194
+ } else if constexpr (__tiny_range<_Pattern>) {
+ // TODO(varconst): should be `ranges::find`.
+ __current() = std::find(std::move(__current()), __end, *__pbegin);
----------------
philnik wrote:
> Quuxplusone wrote:
> > var-const wrote:
> > > Note: this is significant, though (I think) not enough to block this patch. Because the non-ranges version of the algorithm requires both iterators to have the same type, this will fail for ranges where `end()` returns a sentinel. (same applies to `mismatch` below)
> > @philnik already has `mismatch` in progress: D117817
> > I think we //should// block this patch until `ranges::find` is done. It can't be too hard for @philnik or someone else to tackle `find` next, and this way we don't build up a backlog of stuff to remember to go back and fix later.
> `ranges::mismatch` should land within the next couple days. It's just waiting for @ldionne to be happy. I'll start with `ranges::find` next, since it's needed for this.
@philnik Thanks a lot for working on this! I'll wait for everything to land before merging this patch. I have replaced the call to `std::find` with a local duplicate of the actual `std::ranges::find` for now -- `std::find` creates some problems in tests.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:252
+
+ template <bool _Const> struct __inner_iterator : private __inner_iterator_category<__maybe_const<_Const, _View>> {
+ private:
----------------
Quuxplusone wrote:
> Surely this `private` is wrong — that would make the `iterator_category` inaccessible! Can you find a regression test that detects this bug (assuming it's a bug)?
Fixed.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:292
+ return __tmp;
+
+ } else {
----------------
Quuxplusone wrote:
> Remove blank line. (Likewise if there are similar places I didn't comment on, e.g. line 314.)
Why? I'd rather keep it -- it helps visually separate the branches and makes the code less cluttered.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp:42-43
+
+ constexpr std::string_view input = "abc";
+ constexpr std::string_view sep = "a";
+
----------------
philnik wrote:
> Optional, but since you are `using namespace std::string_view_literals` anyways. Or remove it, since you don't seem to use it anywhere.
Removed the `using namespace` -- it was a leftover.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp:47
+
+ // Test `views::lazy_split(input, sep)`.
+ {
----------------
philnik wrote:
> The ``` aren't really useful IMO, but I don't object strongly.
I find them very useful personally, so as long as you don't find them actively harmful I'd rather keep them.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp:107-108
+ struct X { };
+ auto partial = std::views::lazy_split(X{});
+ (void)partial;
+ }
----------------
philnik wrote:
> Optional
Applied throughout.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/constraints.compile.pass.cpp:35
+// All constraints satisfied (`View` and `Pattern` are forward views).
+namespace test1 {
+
----------------
philnik wrote:
> Nit: it would be nice if these namespace names were a bit more descriptive.
Hmm, I got this idea from a pending patch, but I presumed that these namespaces were a nice way to imitate the usual `{}` braces we use to separate test cases. From that perspective, the fact that they're effectively nameless seemed like an advantage, since regular scopes are nameless as well. The comment should describe the purpose of each test case anyway. I'm not strongly opposed, though -- let me know what you think.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.default.pass.cpp:25-28
+LIBCPP_STATIC_ASSERT( std::is_nothrow_default_constructible_v<std::ranges::lazy_split_view<ForwardView, ForwardView>>);
+static_assert(!std::is_nothrow_default_constructible_v<ThrowingDefaultCtorForwardView>);
+static_assert(!std::is_nothrow_default_constructible_v<
+ std::ranges::lazy_split_view<ThrowingDefaultCtorForwardView, ForwardView>>);
----------------
Quuxplusone wrote:
> (1) Why `LIBCPP_STATIC_ASSERT` instead of `static_assert`?
> (2) Please remove the clang-format linebreak between 27-28, for consistency with line 26. Shortening the verbose names might help; moving them into local scopes might help.
(1) I think I wasn't sure whether a "defaulted" constructor is guaranteed to be `noexcept` (assuming member constructors are `noexcept`), but after double-checking I'm pretty sure it is.
(2) I don't think it's a good idea to go out of our way to avoid line breaks. In particular, I think self-explanatory names are a bigger priority than having an expression on a single lines. So I'd rather keep as is.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.range.pass.cpp:27
+concept CanConstruct = requires(R&& r, D&& d) {
+ std::ranges::lazy_split_view<V, P>(std::forward<R>(r), std::move(d));
+};
----------------
Quuxplusone wrote:
> "Forward one but move the other" seems sketchy. I suggest
> ```
> template <class V, class P, class... Args>
> concept CanConstruct = requires (Args&&... args) {
> std::ranges::lazy_split_view<V, P>(std::forward<Args>(args)...);
> };
> ```
> Or even better, move the tests into local scopes and do like
> ```
> {
> using V = std::ranges::lazy_split_view<std::string_view, std::string_view>;
> static_assert( std::is_constructible_v<V, std::string_view, char>);
> static_assert( std::is_constructible_v<V, char(&)[5], char>);
> static_assert(!std::is_constructible_v<V, char(&&)[5], char>);
> ~~~
> }
> ```
Went with the second approach.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/ctor.outer_iterator.pass.cpp:36
+template <class Inner, class Outer>
+constexpr void test_impl() {
+ Inner i(Outer{});
----------------
philnik wrote:
> You could name it `test`, right?
Making a non-template and template functions share the same name seemed unnecessarily confusing when I was writing this. Do you think it would be better?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/types.h:91
+template <class It>
+class almost_forward_iterator {
+ It it_;
----------------
Quuxplusone wrote:
> This appears unused. I recommend combing this file for unused stuff, and then seeing how much of the single-use stuff you can move closer to its place-of-use. Ideally none of our test suite would use this "types.h" pattern.
It is used below (in `ForwardOnlyIfNonConstView`). I will go through this file to see if it can be shortened, though.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107500/new/
https://reviews.llvm.org/D107500
More information about the libcxx-commits
mailing list