[libcxx-commits] [PATCH] D107500: [libc++][ranges] Implement `lazy_split_view`.
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Mar 8 10:29:48 PST 2022
Quuxplusone added a subscriber: philnik.
Quuxplusone added inline comments.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:40
+
+#if !defined(_LIBCPP_HAS_NO_RANGES)
+
----------------
`#if !defined(_LIBCPP_HAS_NO_CONCEPTS) && !defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES)`
(and likewise on the `#endif` of course)
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:118
+
+ template <bool _Const> struct __outer_iterator : __outer_iterator_category<__maybe_const<_Const, _View>> {
+ private:
----------------
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`?
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:133
+ constexpr auto& __current() {
+ if constexpr (forward_range<_Base>) {
+ return __current_;
----------------
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.
================
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);
----------------
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.
================
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:
----------------
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)?
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:292
+ return __tmp;
+
+ } else {
----------------
Remove blank line. (Likewise if there are similar places I didn't comment on, e.g. line 314.)
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:306
+ auto [__pcur, __pend] = ranges::subrange{__x.__i_.__parent_->__pattern_};
+ const auto __end = ranges::end(__x.__i_.__parent_->__base_);
+
----------------
Nit: Remove `const` (the Standard doesn't have it, although conveniently, I also dislike it ;)).
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:329-338
+ friend constexpr decltype(auto) iter_move(const __inner_iterator& __i)
+ noexcept(noexcept(ranges::iter_move(__i.__i_.__current()))) {
+ return ranges::iter_move(__i.__i_.__current());
+ }
+
+ friend constexpr void iter_swap(const __inner_iterator& __x, const __inner_iterator& __y)
+ noexcept(noexcept(ranges::iter_swap(__x.__i_.__current(), __y.__i_.__current())))
----------------
The noexcept-specs here are correct, but `__current() const` itself is missing a `noexcept`, which I believe makes these report the wrong answer. Please add a regression test for each of `iter_swap`, `iter_move`, using `ASSERT_NOEXCEPT`.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:356-358
+ noexcept( noexcept(lazy_split_view(std::forward<_Range>(__range), std::forward<_Pattern>(__pattern))))
+ -> decltype( lazy_split_view(std::forward<_Range>(__range), std::forward<_Pattern>(__pattern)))
+ { return lazy_split_view(std::forward<_Range>(__range), std::forward<_Pattern>(__pattern)); }
----------------
Likewise anywhere else this happens.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:363
+ [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
+ constexpr auto operator()(_Pattern&& __pattern) const
+ noexcept(is_nothrow_constructible_v<decay_t<_Pattern>, _Pattern>) {
----------------
var-const wrote:
> Please check if this is correct. I got this pattern from `std::views::transform`.
Seems reasonable to me. Indent that noexcept-spec on line 364 by 2 or 4 spaces, though. :)
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctad.compile.pass.cpp:49-52
+ // Note: CTAD from (InputRange, TinyForwardRange) doesn't work -- the deduction guide wraps the pattern in
+ // `views::all_t`, resulting in `views::owning_view<TinyForwardRange>`. That type would never satisfy `tiny-range`
+ // because `views::owning_view` contains a member function `size()` that shadows the static `size()` in
+ // `TinyForwardRange`.
----------------
var-const wrote:
> Just wanted to draw attention to this -- it seems like it could be an oversight.
`owning_view::size()` is still constexpr, though... ooh, but it's not //static//, and `tiny-range` for some reason requires the `size` member function to be //static//. Gross. @tcanens, any thoughts?
================
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>>);
----------------
(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.
================
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));
+};
----------------
"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>);
~~~
}
```
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/types.h:91
+template <class It>
+class almost_forward_iterator {
+ It it_;
----------------
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.
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