[libcxx-commits] [PATCH] D107671: [libcxx][ranges] Add `ranges::join_view`.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__ranges/join_view.h:30
+_LIBCPP_PUSH_MACROS
+#include <__undef_macros>
+
----------------
Not needed.


================
Comment at: libcxx/include/__ranges/join_view.h:36-42
+#define _CONSTEXPR_TERNARY(cond, a, b) \
+    [&]() -> auto&& { \
+      if constexpr (cond) \
+        return a; \
+      else \
+        return b; \
+    }();
----------------
If you agree with what I say below, this should be removed.


================
Comment at: libcxx/include/__ranges/join_view.h:80
+    : public view_interface<join_view<_View>>
+    , public __inner_cache<range_reference_t<_View>> {
+  private:
----------------
Instead, I would do:

```
static constexpr bool _UseCache = !is_reference_v<_InnerRange>;
using _Cache = _If<_UseCache, __non_propagating_cache<remove_cv_t<_InnerRange>>, __empty_cache>;
[[no_unique_address]] _Cache __cache_;
```

That way, you get rid of `__inner_cache` altogether.


================
Comment at: libcxx/include/__ranges/join_view.h:84
+
+    template<bool _Const> struct __iterator
+      : public __join_view_iterator_category<__maybe_const<_Const, _View>> {
----------------
`__iterator` and `__sentinel` are templates -- can we define them out-of-line?


================
Comment at: libcxx/include/__ranges/join_view.h:101
+    private:
+      optional<_Inner> __inner_; // TODO: lwg issue?
+      _Parent *__parent_ = nullptr;
----------------
I think you should remove the TODO since this is https://wg21.link/LWG3569 and it will almost certainly be voted into the Standard.


================
Comment at: libcxx/include/__ranges/join_view.h:106-116
+        for (; __outer_ != ranges::end(__parent_->__base_); ++__outer_) {
+          auto&& __inner = _CONSTEXPR_TERNARY(__ref_is_glvalue,
+            *__outer_,
+            __parent_->__inner_.__emplace_deref(__outer_));
+          __inner_ = ranges::begin(__inner);
+          if (*__inner_ != ranges::end(__inner))
+            return;
----------------
IMO the spec is easier to understand as-is, and it doesn't require introducing a `_CONSTEXPR_TERNARY` macro that is only used in two places.

I don't mind if you use a IIFE or not.


================
Comment at: libcxx/include/__ranges/join_view.h:109
+            *__outer_,
+            __parent_->__inner_.__emplace_deref(__outer_));
+          __inner_ = ranges::begin(__inner);
----------------
I don't understand why the spec allows modifying a member of the parent `join_view` from the iterator. That's very sneaky and can lead to e.g. race conditions if someone does:

```
join_view v = ...;
auto it1 = v.begin();
auto it2 = v.begin();

std::thread([=]{
  ++it1; // calls satisfy() under the hood, which modifies `v`
});

std::thread([=]{
  ++it2; // calls satisfy() under the hood, which modifies `v`
});

// sneaky race condition since you had a copy of different iterators in each thread
```

Basically, I think it's quite vexing that incrementing an iterator modifies the underlying view in this case, and I don't fully understand why that is even required. What use case does that enable? Is the criteria `is_reference_v<range_reference_t<V>>` meant to mean something like "the outer range creates its inner ranges on-the-fly and they are temporary objects"? I'm having trouble following here.

CC @tcanens in case you can share some insight here.


================
Comment at: libcxx/include/__ranges/join_view.h:136-137
+      _LIBCPP_HIDE_FROM_ABI
+      __iterator() requires default_initializable<_Outer> &&
+                            default_initializable<_Inner> = default;
+
----------------
You don't need the constraint anymore, since you're storing an optional and never default-constructing your inner iterator.

That isn't what the proposed resolution of https://wg21.link/LWG3569 says right now, but I'll send an email to LWG to request a change to the proposed resolution, since I think it's likely just an oversight.

Can you please add tests where the inner iterator is not default constructible to make sure that we can default construct the `join_view::__iterator`? Basically what we'd add if we were fixing the LWG issue after having implemented `join_view` per the current spec.


================
Comment at: libcxx/include/__ranges/join_view.h:181
+      constexpr void operator++(int) {
+        return ++*this;
+      }
----------------
I think this should not be `return ++*this`, but merely `++*this`. Otherwise, if `++*this` returns something else than `void`, you'll get an error like  `void function '<name>' should not return a value`.

Please add a test that exercises that.


================
Comment at: libcxx/include/__ranges/join_view.h:205-207
+        while (*__inner_ == ranges::begin(*__outer_)) {
+          __inner_ = ranges::end(*--__outer_);
+        }
----------------



================
Comment at: libcxx/include/__ranges/join_view.h:250
+
+    template<bool _Const> struct __sentinel {
+      template<bool> friend struct __sentinel;
----------------
Please define out-of-line.


================
Comment at: libcxx/include/__ranges/join_view.h:286
+    _LIBCPP_HIDE_FROM_ABI
+    constexpr explicit join_view(_View __base)
+      : __base_(_VSTD::move(__base)) {}
----------------
Make sure you test `explicit`-ness.


================
Comment at: libcxx/include/__ranges/join_view.h:313-315
+      if constexpr (forward_range<_View> &&
+                    is_reference_v<_InnerRange> && forward_range<_InnerRange> &&
+                    common_range<_View> && common_range<_InnerRange>)
----------------
This simple re-formatting makes the similarity between this and the `const` version below more obvious.


================
Comment at: libcxx/include/__ranges/join_view.h:327-331
+      if constexpr (forward_range<const _View> &&
+                    is_reference_v<range_reference_t<const _View>> &&
+                    forward_range<range_reference_t<const _View>> &&
+                    common_range<const _View> &&
+                    common_range<range_reference_t<const _View>>) {
----------------
Same reason as above, this makes the similarity between the `const` and the non-`const` versions more obvious.


================
Comment at: libcxx/include/__ranges/join_view.h:340
+  template<class _Range>
+  explicit join_view(_Range&&) -> join_view<views::all_t<_Range>>;
+
----------------
Please make sure you test the explicit-ness of this deduction guide with something like that:

```
join_view v = some_range; // should not compile
join_view v(some_range); // should compile
```


================
Comment at: libcxx/include/__ranges/non_propagating_cache.h:89-95
+    template<class _Other>
+    _LIBCPP_HIDE_FROM_ABI
+    constexpr _Tp& __emplace_deref(const _Other& __value) {
+      __value_.reset();
+      __value_.emplace(*__value);
+      return *__value_;
+    }
----------------
tcanens wrote:
> This isn't right. See https://github.com/microsoft/STL/pull/2038#discussion_r683086306
I'll make this change in a separate patch, I think I have something that works in a WIP branch right now. Thanks for bringing this to our attention.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/end.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
This is a general comment not only applicable to this test. We seem to be missing coverage for a lot of range "topologies" (what I mean by that is an empty range, a range with 1 subrange, 2 subranges, an empty subrange, subranges of empty sizes, etc..).

I don't think it is reasonable to replicate each test for all topologies, but we should add tests with something like a random access range (const and non-const) checking all topologies. What I mean here is that I don't care if you don't test a range like `[[x], [], [y, z]]` with all combinations of archetypes, but you should test it at least with a few that are relevant (that probably is restricted to const and non-const).


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/sentinel/ctor.parent.pass.cpp:11
+// UNSUPPORTED: libcpp-no-concepts
+// UNSUPPORTED: gcc-10
+// UNSUPPORTED: libcpp-has-no-incomplete-ranges
----------------
Not needed (here and everywhere else).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107671