[libcxx-commits] [PATCH] D123466: [libcxx][ranges] add views::join adaptor object. added test coverage to join_view
Hui via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Apr 13 00:40:30 PDT 2022
huixie90 marked 9 inline comments as done.
huixie90 added inline comments.
================
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:
> 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.
`decltype(inner)` is `Inner[3]`, but `decltype((inner))` is `Inner (&) [3]`, which is an lvalue reference. In expression `inners | views::join`, `inners` is lvalue reference
================
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))>);
+ }
----------------
huixie90 wrote:
> var-const wrote:
> > 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.
> `decltype(inner)` is `Inner[3]`, but `decltype((inner))` is `Inner (&) [3]`, which is an lvalue reference. In expression `inners | views::join`, `inners` is lvalue reference
This doesn't add additional coverage. However, this was suggested by @philnik, to check that the local helper concept `CanBePiped` is able to return `true`. (as supposed `concept CanBePiped = false` that renders the negative test useless
================
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));
----------------
var-const wrote:
> Is it possible to check a) whether an iterator or a sentinel is returned; b) whether the returned iterator/sentinel is const?
unfortunately `iterator` and `sentinel` are private exposition only classes. It is not directly accessible. Therefore these tests are testing the type of them indirectly.
For example, to test `end` returns an `iterator`, we can check
`std::ranges::common_range<decltype(jv)>` is `true`, because `common_range` means `begin` and `end` returns the same type.
if the above is false, it means we have an `sentinel`, to test `end()` returns `sentinel<false>` as oppose to `sentinel<true>`, we can check `!std::same_as<decltype(jv.end()), decltype(std::as_const(jv).end())>`
================
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())>);
----------------
var-const wrote:
> Because you already call `std::as_const(jv).end()` above, this check seems redundant (applies throughout -- only the negative checks are necessary).
again, this was suggested by philnik to make sure our local `HasConstEnd` isn't always returning `false`
================
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)>);
----------------
var-const wrote:
> Question: what is the purpose of this check?
see the above explanation about how to check it returns `iterator<true>`, `iterator<false`, `sentinel<true>`, `sentinel<false`
================
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())>);
}
----------------
var-const wrote:
> This check seems redundant -- the call to `jv.begin()->x` above already seems to check that the iterator has `operator->`.
again, happy to remove. was just to double check if `HasArrow` is doing what it is suppose to doing
================
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()) {
----------------
var-const wrote:
> Why is this necessary?
for some reason, gcc fails the compile time test (fine at runtime). I think it is a gcc bug ( i need to find a minimum reproducible example)
================
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);
----------------
var-const wrote:
> Can you check the iterator type?
the `iterator` is a private class and it is not straightforward to test the actual type. any suggestions?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/iter.move.pass.cpp:45
+
+ auto it2 = std::ranges::next(it, 4);
+
----------------
var-const wrote:
> Question: why is checking `iter_move` with another iterator necessary?
If `join_view::iterator` completely forgot to implement `iter_move`, the default implementation of `std::ranges::iter_move` would actually call `std::move(*it)`. The original test point would pass anyway (`std::move(int&) == int&&`)
Here I am adding this additional test with a custom underlying iterator where `iter_move` isn't simply `std::move(*it)`, and also it has got an int to track if its own `iter_move` customisation is called
================
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>);
----------------
var-const wrote:
> What is the reason for this change?
On another review, @philnik was skeptical about the macro `ASSERT_SAME_TYPE`. Here is his comment
> I'm not a huge fan of ASSERT_SAME_TYPE in general. It seems to me like a macro that is there for a very small amount of convenience and makes the code much harder to read IMO. At least I haven't seen a case where it makes life a lot easier.
I was following his suggestion to not to use it. happy to revert. I am ok with either way. what is your opinion?
================
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>);
}
----------------
var-const wrote:
> Seems redundant with the check a couple lines above.
again, it is philnik's suggestion to double check the `HasIterCategory` is not always returning `false`
================
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>;
----------------
var-const wrote:
> Question: why is this type used here?
This test point it to check the `value_type` is defined correctly. This particular type's `value_type` isn't simply `remove_cvref<reference>`. I can add additional `static_assert` 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>>);
}
----------------
var-const wrote:
> Question: can this check be more specific?
could you please elaborate? This test point is checking `difference_type` is defined correctly, which should be the common_type of the inner and outer `difference_type`
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:143
+template <class T>
+struct Buffer{
+ T* buffer_;
----------------
var-const wrote:
> From the name, I would expect a buffer to own the memory, whereas this is non-owning view. Can you use `std::span`?
I will rename it. I'd like to have access to the underlying pointer and I don't think `std::span` has that property
================
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_);
----------------
var-const wrote:
> Nit: move the brace to the previous line? (applies below as well)
sure I will try my best. In general, I use clang-format to format document. It is almost impossible to format the new code to be the same style as the existing code base.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:159
+ {
+ return NonConstIter(this->buffer_);
+ }
----------------
var-const wrote:
> 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_)`.
`this` is required because the base class is a template
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:169
+ } else {
+ return NonConstSent(NonConstIter(this->buffer_ + this->size_));
+ }
----------------
var-const wrote:
> 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).
I still prefer to separate them. It is clear that in one case, we want to have a `common_range` and in another case, we want a `!common_range`
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:286
+template <class It>
+struct copy_iterator
+{
----------------
var-const wrote:
> What is the purpose of this class?
It is an `iterator` where its `reference` is a prvalue. It is used in some of the tests where the inner range produces prvalue `reference`
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:350
+template <class Base>
+struct move_only_input_iter {
+ Base it_;
----------------
var-const wrote:
> 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.
yes. will rename it, it is used in the arrow test.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:375
+template <class Base>
+struct move_iter_sentinel {
+ Base it_;
----------------
var-const wrote:
> Question: why can't `sentinel_wrapper` be used here?
IIRC, `sentinel` requires `semiregular`. However, `sentinel_wrapper` simply delegates to the iterator, which will have a deleted copy constructor in this case, which does not model `semiregular`
================
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_; }
+};
----------------
var-const wrote:
> Question: can this be `= default`?
I think it is not possible? They are two different types (sentinel and iterator)
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