- added views::join adaptor object
- added test for the adaptor object
- fixed some join_view's tests. e.g iter_swap test
- added some negative tests for join_view to test that operations do not exist when constraints aren't met
- added tests that locks down issues that were already addressed in previous change
- LWG3500 join_view::iterator::operator->() is bogus
- LWG3313 join_view::iterator::operator-- is incorrectly constrained
- LWG3517 join_view::iterator's iter_swap is underconstrained
- P2328R1 join_view should join all views of ranges
- fixed some issues in join_view and added tests
- LWG3535 join_view::iterator::iterator_category and ::iterator_concept lie
- LWG3474 Nesting `join_views` is broken because of CTAD
- added tests for an LWG issue that isn't resolved in the standard yet, but the previous code has workaround.
- LWG3569 Inner iterator not default_initializable
Details
- Reviewers
philnik ldionne • Quuxplusone var-const - Group Reviewers
Restricted Project - Commits
- rG3d3103b733d4: [libcxx][ranges] add views::join adaptor object. added test coverage to…
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Could you please update the status document to show which LWG issues you added tests for? And also it would be nice to list them in the commit message!
@ldionne sure. I have updated the review summary, the status doc. Btw, by searching join_view in those docs, I actually found another issue (about iterator_category and iterator_concept) which wasn't fixed previously so I updated the join_view to fix that issue.
Thanks a lot for working on this! The fixes and additional testing look great.
libcxx/include/__ranges/join_view.h | ||
---|---|---|
344 | 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) | |
libcxx/test/std/ranges/range.adaptors/range.join.view/adaptor.pass.cpp | ||
38 | Nit: I think concepts are usually indented 2 (sometimes 4) spaces in our code base, I'd move this line way left. | |
51 | 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. | |
67 | Ultranit: extra backticks. | |
70 | Question: why not use same_as like above? | |
86 | Question: why are the double parentheses necessary? (here and below) | |
86 | 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. | |
libcxx/test/std/ranges/range.adaptors/range.join.view/begin.pass.cpp | ||
33 | Can you add static_asserts to verify the important properties of this class? | |
123 | 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) | |
129 | Optional: consider static asserting !is_reference_v<range_reference_t<std::as_const<decltype(innerRValueRange)>>>. | |
libcxx/test/std/ranges/range.adaptors/range.join.view/ctor.default.pass.cpp | ||
43 | Please add a brief comment explaining the purpose of the test. | |
libcxx/test/std/ranges/range.adaptors/range.join.view/end.pass.cpp | ||
61 | Is it possible to check a) whether an iterator or a sentinel is returned; b) whether the returned iterator/sentinel is const? | |
63–66 | Because you already call std::as_const(jv).end() above, this check seems redundant (applies throughout -- only the negative checks are necessary). | |
65 | Question: what is the purpose of this check? | |
libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/arrow.pass.cpp | ||
29 | Optional: this seems a little overkill for a test-local class. | |
59 | This check seems redundant -- the call to jv.begin()->x above already seems to check that the iterator has operator->. | |
75 | Can you add a little more context to the comment (here and below) to explain what exactly is being checked? | |
libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/ctor.other.pass.cpp | ||
65–72 | This check seems redundant -- the code above already constructed a const_iterator from a non-const iterator. | |
68 | 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. | |
libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/ctor.parent.outer.pass.cpp | ||
19 | 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? | |
37 | Nit: can you expand a little to let the reader know what the test is checking without opening the issue? | |
38 | Optional nit: deduce the size? | |
libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/iter.move.pass.cpp | ||
45 | Question: why is checking iter_move with another iterator necessary? | |
libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/iter.swap.pass.cpp | ||
64 | Can you please expand the comment to provide a little more context -- does this test check that the LWG issue is fixed? | |
libcxx/test/std/ranges/range.adaptors/range.join.view/sentinel/ctor.other.pass.cpp | ||
34 | Can you add a short comment explaining what this test does? | |
libcxx/test/std/ranges/range.adaptors/range.join.view/types.h | ||
468 | From the name, I would expect a buffer to own the memory, whereas this is non-owning view. Can you use std::span? | |
481 | Nit: wrong indentation (1 space instead of 2 spaces IIRC). | |
483 | Nit: move the brace to the previous line? (applies below as well) | |
484 | 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_). | |
494 | 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). | |
508 | Can you add a brief comment explaining the purpose of this class? | |
556 | Nit: looks like there's an extra blank line (2 blank lines in a row). | |
611 | What is the purpose of this class? | |
612 | Formatting nit: move the brace to the previous line. | |
618 | s/referece/reference/. Is this alias necessary? Also, can you add a comment to explain why it's not actually a reference type? | |
632 | Would == default work? | |
647 | Nit: add a space after the comma. | |
675 | 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. | |
697 | Also assert that this class is an input_iterator? | |
700 | Question: why can't sentinel_wrapper be used here? | |
703 | Nit: extra space. | |
704 | Question: can this be = default? | |
719 | Nit: add a space before the opening brace. | |
722 | Add brief comments to explain why these reference aliases are just value types? | |
727 | Given that this class is default-constructible, should these be initialized to nullptr? |
libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/decrement.pass.cpp | ||
---|---|---|
87 | Why is this necessary? | |
104 | Can you check the iterator type? | |
108 | Nit: can this avoid mixing same_as and static_assert and just use one of them consistently? | |
116 | 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. | |
141 | 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? | |
libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/member_types.compile.pass.cpp | ||
59–63 | What is the reason for this change? | |
63 | Seems redundant with the check a couple lines above. | |
85 | Same nit as other similar comments (please add more context). | |
85 | Question: why is this type used here? | |
96 | Question: can this check be more specific? |
libcxx/test/std/ranges/range.adaptors/range.join.view/adaptor.pass.cpp | ||
---|---|---|
86 | 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 | |
86 | 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 | |
libcxx/test/std/ranges/range.adaptors/range.join.view/end.pass.cpp | ||
61 | 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 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())> | |
63–66 | again, this was suggested by philnik to make sure our local HasConstEnd isn't always returning false | |
65 | see the above explanation about how to check it returns iterator<true>, iterator<false, sentinel<true>, sentinel<false | |
libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/arrow.pass.cpp | ||
59 | again, happy to remove. was just to double check if HasArrow is doing what it is suppose to doing | |
libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/decrement.pass.cpp | ||
87 | 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) | |
104 | the iterator is a private class and it is not straightforward to test the actual type. any suggestions? | |
libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/iter.move.pass.cpp | ||
45 | 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 | |
libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/member_types.compile.pass.cpp | ||
59–63 | On another review, @philnik was skeptical about the macro ASSERT_SAME_TYPE. Here is his comment
I was following his suggestion to not to use it. happy to revert. I am ok with either way. what is your opinion? | |
63 | again, it is philnik's suggestion to double check the HasIterCategory is not always returning false | |
85 | 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 | |
96 | 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 | |
libcxx/test/std/ranges/range.adaptors/range.join.view/types.h | ||
468 | I will rename it. I'd like to have access to the underlying pointer and I don't think std::span has that property | |
483 | 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. | |
484 | this is required because the base class is a template | |
494 | 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 | |
611 | 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 | |
675 | yes. will rename it, it is used in the arrow test. | |
700 | 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 | |
704 | I think it is not possible? They are two different types (sentinel and iterator) |
libcxx/test/std/ranges/range.adaptors/range.join.view/adaptor.pass.cpp | ||
---|---|---|
38 | lol. I adopted your suggestion but arc linter put the indentation back. what is the command to disable arc linter? |
libcxx/test/std/ranges/range.adaptors/range.join.view/adaptor.pass.cpp | ||
---|---|---|
38 | --nolint |
This generally looks really good -- I don't have much to add TBH. Thanks a lot for increasing the coverage and being diligent in your testing, this is really valuable.
One thing I would suggest (for the future) would be to split large reviews into a few orthogonal patches. Each LWG issue could have been addressed on its own, same for the added testing and the join adaptor. That being said, don't split it off now since this has gotten reviewed. But in the future, splitting it into orthogonal patches when possible will generally make it easier for reviewers to give you good feedback quickly.
I don't have any blocking comments, so I'm happy when the other reviewers are. Thanks a lot for the contribution, this is great!
libcxx/docs/Status/Cxx2bIssues.csv | ||
---|---|---|
47 | ||
59 | ||
libcxx/test/std/ranges/range.adaptors/range.join.view/adaptor.pass.cpp | ||
12–13 | ||
38 | Specifically, you can upload your patch with arc diff --nolint. That should really be the default given how noisy it is :-) | |
74 | Those can be simplified as ASSERT_SAME_TYPE(std::ranges::range_reference_t<decltype(jv2)>, Foo&);. Non-blocking. | |
libcxx/test/std/ranges/range.adaptors/range.join.view/end.pass.cpp | ||
28 | We haven't been doing this very often, but I think it's a really neat and concise way of making sure we're exhaustive. I think we should start doing this more often when it makes sense. | |
libcxx/test/std/ranges/range.adaptors/range.join.view/types.h | ||
80–82 | It should be sufficient to use sentinel_wrapper in order to remove those (and below). | |
372 | If some of those are only used in a single test file, I would consider moving them to that test file in order to keep this one smaller, and keep things as localized as possible. I think this is the case here, and IMO the test in arrow.pass.cpp would be a bit easier to understand if we defined this type in arrow.pass.cpp, closer to where it is used. For other types that are reused across multiple tests, I think it's worth avoiding code duplication of course. Non-blocking. |
Thanks for addressing the feedback (and sorry about the slow review)! Almost LGTM with just a few nits/questions.
libcxx/test/std/ranges/range.adaptors/range.join.view/adaptor.pass.cpp | ||
---|---|---|
70 | Seems not done, unless I'm missing something. | |
86 | Thanks for explaining, makes sense. | |
libcxx/test/std/ranges/range.adaptors/range.join.view/ctor.default.pass.cpp | ||
32 | Shouldn't it be value initialise? Default initialization would leave i uninitialized. | |
libcxx/test/std/ranges/range.adaptors/range.join.view/end.pass.cpp | ||
61 | Optional: maybe create helper type traits or concepts to make this more obvious? E.g. is_sentinel, is_const_iterator, etc. Feel free to ignore. | |
libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/arrow.pass.cpp | ||
76 | Ultranit: I think should not be defined would be a little clearer, but it's a really small thing, so feel free to ignore (below as well). | |
libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/ctor.parent.outer.pass.cpp | ||
40 | Nit: s/anner/inner/? | |
libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/decrement.pass.cpp | ||
87 | Please add a comment explaining the issue (if you already have filed a GCC bug or of it's easy to do, add a link to the comment, but I wouldn't block this patch on filing the GCC bug). In general, when doing this sort of compiler- or platform-specific workarounds, it's always helpful to leave a comment explaining the issue. | |
libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/iter.move.pass.cpp | ||
45 | Sorry, what I meant is, why are both iter_move_called_times1 and iter_move_called_times2 necessary? What case does iter_move_called_times2 cover that isn't covered by iter_move_called_times1? | |
libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/member_types.compile.pass.cpp | ||
59–63 | Thanks for explaining. Re. ASSERT_SAME_TYPE -- personally, I actually find it a little easier to read with less boilerplate, but it's definitely not worth it to go and change it back now. I don't think there ever was a general agreement to stop using ASSERT_SAME_TYPE, so I think it comes down to personal preference (@philnik, please correct me if I'm wrong on this). | |
96 | Never mind, on a second thought I think the current state is reasonable (I was going to suggest using a more specific type but it would be non-portable, and trying to work around that is not worth the trouble). | |
libcxx/test/std/ranges/range.adaptors/range.join.view/types.h | ||
365 | Nit: I found this part (about zip_view) a little confusing, can you elaborate? If you mean that zip_view will have a similar move_swap_aware_iter type, then it's better to just omit it -- currently, the test code for different classes is written to be self-contained. | |
468 |
Wouldn't data() work? | |
611 | Thanks for explaining. Optionally, consider renaming it to copying_iterator. |
address comments
libcxx/test/std/ranges/range.adaptors/range.join.view/adaptor.pass.cpp | ||
---|---|---|
70 | Could you please be more explicitly about which line is missing? I think I changed all the things to std::same_as<expected_type> decltype(auto) x = ...; Line 72 and 75 are not checking the type of the variable but the range_reference_t of the type of the variable. | |
libcxx/test/std/ranges/range.adaptors/range.join.view/end.pass.cpp | ||
61 | thanks for the suggestion, I did consider to come up with better helper concepts but it is not trivial to come up with one. The concept cannot simple take just one iterator type like template <class T> concept is_sentinel = ... because there is no easy way to test if a type is the sentinel type. it will probably end of with something like template <class T, class Iterator> concept is_sentinel = !same_as<T, Iterator> which checks if T is the same type as the iterator. but then this concept seems to be a bit less useful, as it is basically the common_range template <class R> concept common_range = same_as<range_iterator_t<R>, range_sentinel_t<R>> any suggestions? | |
libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/iter.move.pass.cpp | ||
45 | since I am joining a range of 2 inner ranges, this is just to make sure that the iter_move is called on the iterator of the correct inner range after each step. If you think this is unnecessary I can delete it and just testing joining a range of 1 inner range | |
libcxx/test/std/ranges/range.adaptors/range.join.view/types.h | ||
365 | this is to test joining a range of zip_view without actually having zip_view. This iterator is emulating proxy-like iterator where value_type and reference do not simply have the remove_cvref relation. zip_view's iterator is just one of this kind. The test is not testing zip_view but joining a range of zip-like ranges | |
468 | that Span/Buffer class is deleted now. Now I only have the BufferView template where its template parameters are iterators and sentinels. |
LGTM (with just a couple of really small comments). Thanks!
libcxx/test/std/ranges/range.adaptors/range.join.view/adaptor.pass.cpp | ||
---|---|---|
70 | Ah, I missed that the type is adjusted in this case, thanks for pointing it out. | |
libcxx/test/std/ranges/range.adaptors/range.join.view/end.pass.cpp | ||
61 |
I think the main utility of this concept would be its descriptive name, so even as an alias for common_range it could have some value (it's local to this file so it's fine for this concept to be not very useful in general). But I don't want to block this patch on this -- feel free to keep things as they are. | |
libcxx/test/std/ranges/range.adaptors/range.join.view/iterator/iter.move.pass.cpp | ||
45 | It makes sense -- maybe add a short comment to that effect, though. | |
libcxx/test/std/ranges/range.adaptors/range.join.view/types.h | ||
365 | How about the following rephrasing? // This is a proxy-like iterator where `reference` is a value type, and `reference` and `value_type` // are distinct types (similar to `zip_view::iterator`). Though given that, like you say, there are other iterators with similar properties, perhaps the part about it being similar to zip_view is unnecessary -- I think I'd drop it and and just keep the explanation about it being a proxy iterator. |