[libcxx-commits] [PATCH] D123466: [libcxx][ranges] add views::join adaptor object. added test coverage to join_view
Konstantin Varlamov via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Apr 12 21:25:19 PDT 2022
var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.
Thanks a lot for working on this! The fixes and additional testing look great.
================
Comment at: libcxx/include/__ranges/join_view.h:344
+namespace __join_view {
+ struct __fn : __range_adaptor_closure<__fn> {
+ template<class _Range>
----------------
Formatting nit: namespaces don't introduce a new indentation level in the LLVM style (see https://llvm.org/docs/CodingStandards.html#namespace-indentation).
(this doesn't apply to namespace `__cpo` below because it was previously decided to special-case those namespaces)
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/adaptor.pass.cpp:37
+concept CanBePiped = requires(View&& view, T&& t) {
+ { std::forward<View>(view) | std::forward<T>(t) };
+ };
----------------
Nit: I think concepts are usually indented 2 (sometimes 4) spaces in our code base, I'd move this line way left.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/adaptor.pass.cpp:50
+ using Result = std::ranges::join_view<std::ranges::ref_view<ForwardCommonInner[3]>>;
+ std::same_as<Result> auto v = std::views::join(inners);
+ assert(std::ranges::next(v.begin(), 9) == v.end());
----------------
Nit: I'd suggest always using `decltype(auto)` with `same_as` -- plain `auto` can decay and hide that a reference is returned. Now, it's very unlikely to happen here, but I feel consistently using `decltype(auto)` is conceptually simpler (no need to judge each case), always correct and not that much more to type.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/adaptor.pass.cpp:66
+ {
+ // LWG3474 Nesting ``join_views`` is broken because of CTAD
+ // views::join(join_view) should join the view instead of calling copy constructor
----------------
Ultranit: extra backticks.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/adaptor.pass.cpp:69
+ auto jv = std::views::join(nested);
+ static_assert(std::is_same_v<std::ranges::range_reference_t<decltype(jv)>, Foo(&)[3]>);
+
----------------
Question: why not use `same_as` like above?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/adaptor.pass.cpp:85
+ assert(&(*v.begin()) == buffer1);
+ static_assert(CanBePiped<decltype((inners)), decltype((std::views::join))>);
+ }
----------------
Question: why are the double parentheses necessary? (here and below)
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/adaptor.pass.cpp:85
+ assert(&(*v.begin()) == buffer1);
+ static_assert(CanBePiped<decltype((inners)), decltype((std::views::join))>);
+ }
----------------
var-const wrote:
> Question: why are the double parentheses necessary? (here and below)
Does this check provide any additional coverage? It seems like it does the same as `inners | views::join`, except it doesn't exercise the runtime part.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/begin.pass.cpp:33
+struct ConstNotRange : std::ranges::view_base {
+ const ChildView* begin();
----------------
Can you add `static_assert`s to verify the important properties of this class?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/begin.pass.cpp:109
+ {
+ // !input_range<const V>
+ std::ranges::join_view jv{ConstNotRange{}};
----------------
Nit: can you please move the comment to come just before the scope, for consistency with the rest of the file? (and also add an empty line in-between the new test cases)
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/begin.pass.cpp:115
+ // !is_reference_v<range_reference_t<const V>>
+ auto innerRValueRange = std::views::iota(0, 5) | std::views::transform([](int) { return ChildView{}; });
+ std::ranges::join_view jv{innerRValueRange};
----------------
Optional: consider static asserting `!is_reference_v<range_reference_t<std::as_const<decltype(innerRValueRange)>>>`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/ctor.default.pass.cpp:31
+ }
+ {
+ std::ranges::join_view<DefaultView> jv;
----------------
Please add a brief comment explaining the purpose of the test.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/end.pass.cpp:61
assert(jv.end() == std::ranges::next(jv.begin(), 16));
+ assert(std::as_const(jv).end() == std::ranges::next(std::as_const(jv).begin(), 16));
----------------
Is it possible to check a) whether an iterator or a sentinel is returned; b) whether the returned iterator/sentinel is const?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/end.pass.cpp:63
- static_assert(std::same_as<decltype(jv.end()), decltype(jv.begin())>);
+ static_assert(HasConstEnd<decltype(jv)>);
+ static_assert(std::same_as<decltype(jv.end()), decltype(std::as_const(jv).end())>);
----------------
Because you already call `std::as_const(jv).end()` above, this check seems redundant (applies throughout -- only the negative checks are necessary).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/end.pass.cpp:65
+ static_assert(std::same_as<decltype(jv.end()), decltype(std::as_const(jv).end())>);
+ static_assert(std::ranges::common_range<decltype(jv)>);
+ static_assert(std::ranges::common_range<const decltype(jv)>);
----------------
Question: what is the purpose of this check?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/arrow.pass.cpp:29
+ arrow_input_iter()
+ requires std::default_initializable<Base>
+ = default;
----------------
Optional: this seems a little overkill for a test-local class.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/arrow.pass.cpp:57
assert(jv.begin()->x == 1111);
+ static_assert(HasArrow<decltype(jv.begin())>);
}
----------------
This check seems redundant -- the call to `jv.begin()->x` above already seems to check that the iterator has `operator->`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/arrow.pass.cpp:73
+ {
+ // LWG3500 `join_view::iterator::operator->()` is bogus
+ // has-arrow<InnerIter> && !copyable<InnerIter>
----------------
Can you add a little more context to the comment (here and below) to explain what exactly is being checked?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/ctor.other.pass.cpp:48
- CopyableChild children[4] = {CopyableChild(buffer[0]), CopyableChild(buffer[1]), CopyableChild(buffer[2]), CopyableChild(buffer[3])};
- std::ranges::join_view jv(CopyableParent{children});
- auto iter1 = jv.begin();
- std::ranges::iterator_t<const decltype(jv)> iter2 = iter1;
- assert(iter1 == iter2);
+ static_assert(std::constructible_from<const_iterator, iterator>);
+ // We cannot create a non-const iterator from a const iterator.
----------------
This check seems redundant -- the code above already constructed a `const_iterator` from a non-const `iterator`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/ctor.other.pass.cpp:51
+ static_assert(!std::constructible_from<iterator, const_iterator>);
+ }
+ {
----------------
General nit: it would be easier to see where one test case ends and another one starts if they were additionally separated by empty lines.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/ctor.parent.outer.pass.cpp:19
+using NonDefaultIter = cpp20_input_iterator<int*>;
+static_assert(!std::default_initializable<NonDefaultIter>);
----------------
Nit: it's not obvious (to me, at least) from the name that "default" refers to the default constructor. How about something like `NoDefaultCtrIter` or `NotDefaultCtrableIter`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/ctor.parent.outer.pass.cpp:37
+ {
+ // LWG 3569 Inner iterator not default_initializable
+ NonDefaultIterView inners[2] = {buffer[0], buffer[1]};
----------------
Nit: can you expand a little to let the reader know what the test is checking without opening the issue?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/ctor.parent.outer.pass.cpp:38
+ // LWG 3569 Inner iterator not default_initializable
+ NonDefaultIterView inners[2] = {buffer[0], buffer[1]};
+ auto outer = std::views::all(inners);
----------------
Optional nit: deduce the size?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/decrement.pass.cpp:77
+ {
+#if defined(__GNUG__) && !defined(__clang__)
+ if (!std::is_constant_evaluated()) {
----------------
Why is this necessary?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/decrement.pass.cpp:94
+ std::ranges::join_view jv(buffer);
+ auto iter1 = std::ranges::next(jv.begin(), 4);
+ using iterator = decltype(iter1);
----------------
Can you check the iterator type?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/decrement.pass.cpp:98
+ decltype(auto) iter2 = --iter1;
+ static_assert(std::is_same_v<decltype(iter2), iterator&>);
+ assert(&iter1 == &iter2);
----------------
Nit: can this avoid mixing `same_as` and `static_assert` and just use one of them consistently?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/decrement.pass.cpp:106
+ // !ref-is-glvalue
+ BidiCommonInner inners[2] = {buffer[0], buffer[1]};
+ InnerRValue<BidiCommonOuter<BidiCommonInner>> outer{inners};
----------------
These 4 tests are very similar -- consider creating a `constexpr` function that takes the initializer of `join_view` as a parameter and asserts that the iterator cannot be decremented.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/decrement.pass.cpp:131
+ {
+ // LWG3313 `join_view::iterator::operator--` is incorrectly constrained
+ // !common_range<range_reference_t<Base>>
----------------
Similar nit to other places -- can you please expand the comment a little bit to make it clear whether this tests for the fixed behavior?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/iter.move.pass.cpp:45
+
+ auto it2 = std::ranges::next(it, 4);
+
----------------
Question: why is checking `iter_move` with another iterator necessary?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/iter.swap.pass.cpp:58
+ {
+ // LWG3517 `join_view::iterator`'s `iter_swap` is underconstrained
+ NonSwappableView inners[2] = {buffer[0], buffer[1]};
----------------
Can you please expand the comment to provide a little more context -- does this test check that the LWG issue is fixed?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/member_types.compile.pass.cpp:59
- ASSERT_SAME_TYPE(Iter::iterator_concept, std::bidirectional_iterator_tag);
- ASSERT_SAME_TYPE(Iter::iterator_category, std::bidirectional_iterator_tag);
- ASSERT_SAME_TYPE(Iter::difference_type, std::ptrdiff_t);
- ASSERT_SAME_TYPE(Iter::value_type, int);
+ static_assert(std::is_same_v<Iter::iterator_concept, std::bidirectional_iterator_tag>);
+ static_assert(std::is_same_v<Iter::iterator_category, std::bidirectional_iterator_tag>);
----------------
What is the reason for this change?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/member_types.compile.pass.cpp:63
+ static_assert(std::is_same_v<Iter::value_type, int>);
+ static_assert(HasIterCategory<Iter>);
}
----------------
Seems redundant with the check a couple lines above.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/member_types.compile.pass.cpp:83
+ {
+ // LWG3535 `join_view::iterator::iterator_category` and `::iterator_concept` lie
+ // Bidi non common inner range should not have bidirectional_iterator_tag
----------------
Same nit as other similar comments (please add more context).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/member_types.compile.pass.cpp:102
+ // value_type == inner's value_type
+ using Inner = IterMoveSwapAwareView;
+ using Outer = BidiCommonOuter<Inner>;
----------------
Question: why is this type used here?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/member_types.compile.pass.cpp:113
+ using Iter = std::ranges::iterator_t<std::ranges::join_view<Outer>>;
+ static_assert(std::is_same_v<Iter::difference_type, std::common_type_t<std::intptr_t, std::ptrdiff_t>>);
}
----------------
Question: can this check be more specific?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/sentinel/ctor.other.pass.cpp:34
+ }
+ {
+ using Inner = BufferView<int*>;
----------------
Can you add a short comment explaining what this test does?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:143
+template <class T>
+struct Buffer{
+ T* buffer_;
----------------
>From the name, I would expect a buffer to own the memory, whereas this is non-owning view. Can you use `std::span`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:156
+
+ constexpr NonConstIter begin()
+ requires(!std::is_same_v<Iter, NonConstIter>)
----------------
Nit: wrong indentation (1 space instead of 2 spaces IIRC).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:158
+ requires(!std::is_same_v<Iter, NonConstIter>)
+ {
+ return NonConstIter(this->buffer_);
----------------
Nit: move the brace to the previous line? (applies below as well)
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:159
+ {
+ return NonConstIter(this->buffer_);
+ }
----------------
Nit: I don't think it's necessary to explicitly mention `this`, and we don't normally do it. This should be just `NonConstIter(buffer_)`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:169
+ } else {
+ return NonConstSent(NonConstIter(this->buffer_ + this->size_));
+ }
----------------
Optional: I think this line can be used unconditionally (it will be `NonConstIter(NonConstIter(...))` if the types are the same, but given that this is test code, I don't think the potential extra object matters).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:183
+template <class Base>
+struct common_input_iterator {
+ Base it_;
----------------
Can you add a brief comment explaining the purpose of this class?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:231
+static_assert(!std::ranges::common_range<BidiNonCommonInner>);
+
+
----------------
Nit: looks like there's an extra blank line (2 blank lines in a row).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:286
+template <class It>
+struct copy_iterator
+{
----------------
What is the purpose of this class?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:287
+struct copy_iterator
+{
+ It it_ = It();
----------------
Formatting nit: move the brace to the previous line.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:293
+ using pointer = typename std::iterator_traits<It>::pointer;
+ using referece = value_type;
+
----------------
`s/referece/reference/`. Is this alias necessary? Also, can you add a comment to explain why it's not actually a reference type?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:307
+
+ friend constexpr bool operator==(const copy_iterator& x, const copy_iterator& y) {
+ return x.it_ == y.it_;
----------------
Would `== default` work?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:322
+ using Outer::Outer;
+ static_assert(std::ranges::common_range<Outer>,"non-common range is not supported yet");
+
----------------
Nit: add a space after the comma.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:350
+template <class Base>
+struct move_only_input_iter {
+ Base it_;
----------------
Question: IIRC, `cpp20_input_iterator` is move-only, so just from reading the name, this class seems unnecessary. However, it also has `operator->`, which I presume is important for how the class is used. Consider renaming and adding a brief comment explaining why this class is necessary.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:372
+};
+static_assert(!std::copyable<move_only_input_iter<int*>>);
+
----------------
Also assert that this class is an `input_iterator`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:375
+template <class Base>
+struct move_iter_sentinel {
+ Base it_;
----------------
Question: why can't `sentinel_wrapper` be used here?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:378
+ explicit move_iter_sentinel() = default;
+ constexpr move_iter_sentinel( Base it) : it_(std::move(it)) {}
+ constexpr bool operator==(const move_only_input_iter<Base>& other) const { return it_ == other.it_; }
----------------
Nit: extra space.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:379
+ constexpr move_iter_sentinel( Base it) : it_(std::move(it)) {}
+ constexpr bool operator==(const move_only_input_iter<Base>& other) const { return it_ == other.it_; }
+};
----------------
Question: can this be `= default`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:394
+
+struct move_swap_aware_iter{
+
----------------
Nit: add a space before the opening brace.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:397
+ using value_type = std::pair<int, int>;
+ using reference = std::pair<int&, int&>;
+ using rvalue_reference = std::pair<int&&, int&&>;
----------------
Add brief comments to explain why these reference aliases are just value types?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:402
+
+ int* iter_move_called;
+ int* iter_swap_called;
----------------
Given that this class is default-constructible, should these be initialized to `nullptr`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123466/new/
https://reviews.llvm.org/D123466
More information about the libcxx-commits
mailing list