Details
- Reviewers
• Quuxplusone ldionne philnik - Group Reviewers
Restricted Project - Commits
- rG55bd22f853d8: [libc++][ranges] Implement rbegin, rend, crbegin and crend.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/include/__ranges/access.h | ||
---|---|---|
260 | "Otherwise, if T is an array type ([term.array.type]) and remove_all_extents_t<T> is an incomplete type, ranges::rbegin(E) is ill-formed with no diagnostic required." | |
267 | "Otherwise, if auto(t.rbegin()) is a valid expression whose type models input_or_output_iterator, ranges::rbegin(E) is expression-equivalent to auto(t.rbegin())." | |
275 | "Otherwise, if T is a class or enumeration type and auto(rbegin(t)) is a valid expression whose type models input_or_output_iterator with overload resolution performed in a context in which unqualified lookup for rbegin finds only the declarations void rbegin(auto&) = delete; void rbegin(const auto&) = delete; then ranges::rbegin(E) is expression-equivalent to auto(rbegin(t)) with overload resolution performed in the above context." | |
283 | "Otherwise, if both ranges::begin(t) and ranges::end(t) are valid expressions of the same type which models bidirectional_iterator ([iterator.concept.bidir]), ranges::rbegin(E) is expression-equivalent to make_reverse_iterator(ranges::end(t))." | |
332 | "Otherwise, if T is an array type ([term.array.type]) and remove_all_extents_t<T> is an incomplete type, ranges::rend(E) is ill-formed with no diagnostic required." | |
339 | "Otherwise, if auto(t.rend()) is a valid expression whose type models sentinel_for<decltype(ranges::rbegin(E))> then ranges::rend(E) is expression-equivalent to auto(t.rend())." | |
347 | "Otherwise, if T is a class or enumeration type and auto(rend(t)) is a valid expression whose type models sentinel_for<decltype(ranges::rbegin(E))> with overload resolution performed in a context in which unqualified lookup for rend finds only the declarations void rend(auto&) = delete; void rend(const auto&) = delete; then ranges::rend(E) is expression-equivalent to auto(rend(t)) with overload resolution performed in the above context." | |
355 | "Otherwise, if both ranges::begin(t) and ranges::end(t) are valid expressions of the same type which models bidirectional_iterator ([iterator.concept.bidir]), then ranges::rend(E) is expression-equivalent to make_reverse_iterator(ranges::begin(t))." | |
libcxx/test/std/ranges/range.access/begin.pass.cpp | ||
91 | I reordered class definitions to be in the order in which they are used in the test function. | |
libcxx/test/std/ranges/range.access/rbegin.pass.cpp | ||
1 | Note: these tests are largely copied from begin.pass.cpp (same with rend.pass.cpp). | |
301 | Note: these tests (up to and including testBeginEnd) are new (same in rend.pass.cpp). |
Thanks for finishing up this clause!
I've reviewed the implementation but not yet the tests. Some significant code comments will lead to adding some extra tests. (And if you find that you want to add analogous regression tests to begin.pass.cpp and end.pass.cpp too, awesome!)
libcxx/include/__ranges/access.h | ||
---|---|---|
16 | Because of this new dependency (which is probably circular, once we get around to implementing C++20 reverse_iterator), and because the four functions you're adding are basically symmetric with the four in this file, please split them out into a new detail header. I suggest rbegin.h (for rbegin/crbegin) and rend.h (rend/crend) if you can get away with it. If that idea runs into problems for some reason, then I'd be reasonably happy with just raccess.h (by symmetry with access.h). | |
224–227 | Note: This could be arguably more simply expressed as template <class _Tp> concept __can_reverse = __can_borrow<_Tp> && same_as<iterator_t<_Tp>, sentinel_t<_Tp>> && bidirectional_iterator<iterator_t<_Tp>>; or even template <class _Tp> concept __can_reverse = __can_borrow<_Tp> && common_range<_Tp> && bidirectional_iterator<iterator_t<_Tp>>; However, the way you wrote it is the closest to the Standard's actual wording (why they used that wording, I don't know) and perhaps marginally faster to compile, so, I think whatever you want to do here is fine. (Alternatively, if you decide to dig into this and discover a way in which either of my rewrites is not equivalent to the Standard's wording, please make sure to add a regression test so that something will fail if we later decide to go with one of those rewrites anyway.) | |
260 | You suggested on D118963 that sizeof(_Tp) != 0 would be better, and I agree with you. Please make the substitution throughout. Also, please order the overloads in the same order as the bullet points in the Standard, just for the benefit of future readers. http://eel.is/c++draft/range.access#rbegin-2 This is bullet point... uh... wait... actually this isn't one of the points at all! The "bounded array of complete type" case is handled in the Standard by point (2.5). I suspect you can dig into this and find a situation where your current overload set produces ambiguity. Something like this maybe? struct Mine { friend Mine *rbegin(Mine *p) { return p; } friend Mine *rend(Mine *p) { return p; } }; Mine a[10]; ... std::ranges::rbegin(a) ... If you find such a case (and I would bet money it's possible to find), then please add it as a regression test. Edit to add: I wonder if std::string[10] is such a case! std::string[10] has an ADL rbegin and rend out of the box. | |
297 | Remove blank line here (and presumably likewise throughout); try to be consistent with the existing code. |
libcxx/include/__ranges/access.h | ||
---|---|---|
283 | I believe your __can_reverse needs to include !__unqualified_rbegin<_Tp> && !__member_rbegin<_Tp> &&, or else you need to include them ad-hoc right here. I have a moderate preference for not ad-hoc, even if it means that you'll have to duplicate the concept into a __can_reverse_begin and a __can_reverse_end for use further down. | |
284 | Attn @ldionne — it's our new policy to start using unadorned std:: in new code, is that right? Or should we keep the _VSTD:: convention in case the release/14.x process discovers an unexpected problem and we need to roll back the #define _VSTD std change? | |
289 | You don't need this — do you? Check whether ranges::begin has it, and be consistent with whatever it is that they do. | |
332 | As above: this overload as written is asking for complete types, which should be handled by points (2.4) and (2.5) instead; so remove this overload. | |
libcxx/test/std/ranges/range.access/begin.pass.cpp | ||
1 | These changes to begin.pass.cpp and end.pass.cpp LGTM, but not directly relevant to this PR. I say go ahead and land these two files (assuming that they still pass after rebasing on trunk), and then this PR will get a bit shorter. Thanks for the cleanup! |
Btw, once the bugs are polished out, I think this would be an obvious candidate to cherry-pick back to release/14.x. It would be really great to be able to say that libc++14 includes all of the iterator CPOs.
@var-const: Please also update libcxx/test/std//library/description/conventions/customization.point.object/cpo.compile.pass.cpp, and git grep rbegin to see if there's any more places that need updating.
Run libcxx-generate-files
libcxx/include/__ranges/access.h | ||
---|---|---|
16 | Done. (Note that in __iterator/, the header structure is access.h/reverse_access.h) | |
224–227 | Thanks for calling it out. For now, I'd prefer to leave this as a copy-paste from the Standard, but I'm interested to play around with whether the two versions are equivalent. I'll let you know what I find. | |
260 |
Done.
Yeah, I was trying to use the same ordering and created an extra clause for handling arrays specifically to satisfy 2.2 (which is otherwise identical to 2.5). Removed it now since like you said it is already handled implicitly.
Hmm, both Mine[10] and std::string[10] compile fine. When I was writing this, I thought there would be no ambiguity because constraints are only checked on functions with the same signature; thus, template argument deduction would always prefer the (_Tp (&)[_Np]) signature over the (_Tp&&) signature, before concepts come into play. I might well be wrong, though. | |
283 | This is a great catch, thanks! Added a test structure (MemberBeginAndRBegin) and made sure it fails without the code change. | |
284 | (Yeah, I did that due to the recent patch/direction to remove _VSTD. Happy to use _VSTD if necessary) | |
289 | It is the same in ranges::begin. I don't know the rationale -- presumably if overload resolution selects a deleted function, it produces a less verbose error than when it cannot find a match. | |
297 | Removed. I don't want to block this patch on this, but in general I prefer to have blank lines. I feel like the opening (or closing) of a namespace and the first (last) element of that namespace don't have any special relationship to warrant grouping them together. | |
libcxx/test/std/ranges/range.access/begin.pass.cpp | ||
1 |
Continues to look pretty much good. Please see what you can do to shorten the tests — 535 lines for rend.pass.cpp seems excessive, for example. I know a big part of that comes from being consistent with the existing end.pass.cpp, though, so it's not a blocker.
libcxx/include/__ranges/rbegin.h | ||
---|---|---|
18 ↗ | (On Diff #406726) | You don't need enable_borrowed_range in this file. (Just the borrowed_range concept itself, which comes from concepts.h). enable_borrowed_range.h is the bare-bones "just the trait" header for people who need to specialize the trait but don't care about querying the concept. I suspect the similar inclusion in __ranges/access.h might also be unused. |
20 ↗ | (On Diff #406726) | Can you be more granular than all of <concepts>? |
24 ↗ | (On Diff #406726) | |
85 ↗ | (On Diff #406726) | |
libcxx/include/__ranges/rend.h | ||
17 ↗ | (On Diff #406726) | also #include <__ranges/rbegin.h> |
87 ↗ | (On Diff #406726) | Consider adding a regression test, although maybe that'll end up looking too silly, once you try it. |
libcxx/test/std/ranges/range.access/rbegin.pass.cpp | ||
125–129 | As in the other review, I'd say that EmptyRBeginMember is redundant with RBeginMemberReturnsInt; consider removing it. | |
301 | remove extra blank line | |
301 | SGTM. Do you have a test for struct EndWithoutBegin { int *end() const; } that proves such a type is not rbegin'able? | |
365–366 | This is an even simpler case, IIUC: both types are iterators, with the same value_type, interconvertible (well, at least in one direction), and yet still they aren't the same type, so the type is not rbegin'able. I consider FunctionBeginEndDifferentTypes redundant with this test. | |
383 | I consider FunctionBeginEndForwardIterators redundant with MemberBeginEndForwardIterators. | |
402 | I consider FunctionEndOnly redundant with MemberEndOnly (I'd just keep the latter); and I'm not sure that MemberBeginOnly and FunctionBeginOnly are actually testing anything worth testing, since "obviously" one can't get a reverse-iterator-to-end-of-range out of nothing but a begin() method. | |
libcxx/test/std/ranges/range.access/rend.pass.cpp | ||
445 | LGTM, but please also test that static_assert(std::same_as<std::invoke_result_t<RangeREndT, MemberBeginAndRBegin&>, int*>); (and not reverse_iterator<int*>). Actually, it would be profitable to replace all your positive std::is_invocable_v assertions with positive std::same_as assertions. It's free test coverage! |
Address review feedback.
libcxx/include/__ranges/rbegin.h | ||
---|---|---|
18 ↗ | (On Diff #406726) | Right. access.h still needs to keep the include (it uses enable_borrowed_range in the definition of __can_borrow). |
20 ↗ | (On Diff #406726) | Done (and it looks like access.h doesn't need <concepts> at all). |
libcxx/include/__ranges/rend.h | ||
87 ↗ | (On Diff #406726) | Thanks for catching this! Added two tests structs (NoThrowBeginThrowingEnd and NoThrowEndThrowingBegin). I think, however, that it has to be just ranges::begin(__t), not std::make_reverse_iterator(ranges::begin(__t)). make_reverse_iterator is not marked noexcept, so IIUC, it will always lead to noexcept(false). At the same time, it doesn't throw any exceptions of its own, thus the actual behavior solely depends on whether ranges::begin(__t) can throw. Does this sound right to you? I can confirm that adding make_reverse_iterator breaks the existing noexcept tests. |
libcxx/test/std/ranges/range.access/rbegin.pass.cpp | ||
301 | Hmm, this is consistently used in the existing begin/end.pass.cpp tests to separate different groups of test cases (basically, two empty lines after every test function). I don't particularly like the style, but I followed it for consistency. If these are to be removed, they should be removed from the existing tests as well. What do you think? | |
301 | I think MemberEndOnly is the equivalent test. | |
365–366 | Replaced char* with const int*. I'd rather keep bidirectional_iterator for consistency with other test cases. | |
383 | See my comment below re. Function/MemberEndOnly. | |
402 | I don't feel particularly strongly about keeping these. My intention was to make sure that all these incorrect cases are silently SFINAE'd away as opposed to leading to compilation errors (and they come two versions, member and free, because these are handled by different overloads). So the idea is not to make sure that these can't work (which, like you said, is pretty obvious), but rather that they fail in the expected quiet fashion. If you still feel this isn't worth testing, I can remove these. | |
libcxx/test/std/ranges/range.access/rend.pass.cpp | ||
445 | Done.
I'm not sure. It does add to the test coverage, but it's also more verbose and, IMO, doesn't reflect the intention quite as well. In any case, if this is to be done, it should also be done in the existing tests, so it seems more suitable for a separate patch. |
Somehow the size of rend.pass.cpp went up since I complained about its size. ;)
Anyway, once the "noexcept/expression-equivalent" thing is fixed, I think this is good to go.
@ldionne LGTY? Thoughts on cherry-picking into release/14.x so we can say we've got all of [range.access] in 14.x?
libcxx/include/__ranges/rend.h | ||
---|---|---|
87 ↗ | (On Diff #406726) | https://eel.is/c++draft/range.access.rbegin#2.5 is 100% clear that ranges::rbegin(t) must be expression-equivalent to std::make_reverse_iterator(ranges::end(t)), which means that the two expressions must have exactly the same noexceptness. If that means that right now today libc++'s ranges::rbegin is never-noexcept, then OK, that's what it means. And yep, The only thing we could do here that would be non-conforming, would be to have make_reverse_iterator non-noexcept and ranges::rbegin noexcept. That's definitely not permitted by the Standard, because it says the two must be expression-equivalent. So, please do the "you must write it three times" thing for now (i.e. noexcept(noexcept(the-exact-expression-you're-going-to-return))). And please also triple-check the rest of your noexcept-specifications to make sure they're all correct by this logic. |
libcxx/test/std/ranges/range.access/rbegin.pass.cpp | ||
402 | Yeah, I'd prefer to see them removed. The place for the silent-SFINAE test is end.pass.cpp/begin.pass.cpp, and I'm 99% sure we've got those tests over there. | |
libcxx/test/std/ranges/range.access/rend.pass.cpp | ||
445 |
Well, maybe it changes the perceived intention. Instead of a bunch of similar tests — "this invocation compiles, this one doesn't compile..." — it gives us a bunch of similar tests plus a few "outlier" cases — "this invocation gives T, this invocation gives U, this one doesn't even compile, this invocation gives V..." |
This would be disabled due to it being incomplete anyway, so I don't see the point in cherry-picking it back to 14.x.
libcxx/include/__ranges/access.h | ||
---|---|---|
284 | Yes, we can use std::, but thanks for checking. | |
libcxx/include/__ranges/rbegin.h | ||
28 ↗ | (On Diff #407044) | This should be guarded by HAS_NO_INCOMPLETE_RANGES too (and other headers too). |
libcxx/include/__ranges/rbegin.h | ||
---|---|---|
28 ↗ | (On Diff #407044) | Yes (perhaps), but that's D118736; I think as long as this header is consistent with what we currently do, it's better not to introduce potential churn over it here. My impression of D118736 is that we're still kinda confused about what we want HAS_NO_INCOMPLETE_RANGES to encompass — I believe in my original PR it would have been included because it's in ranges::; and then in my subsequent tentative proposal it wouldn't have been because it has no ABI implications (no struct layout); and then in your current proposal it would again be included because it's in ranges:: and no std:: concept depends on it. I assume we'll talk about that offline soon. :) |
Address review feedback:
- fix noexcept and the return types in the overloads that return std::make_reverse_iterator;
- adjust tests to expect this overload to always not be noexcept;
- remove redundant tests.
libcxx/include/__ranges/rbegin.h | ||
---|---|---|
28 ↗ | (On Diff #407044) | My reasoning was pretty much what @Quuxplusone said -- until D118736 lands, I think new code should follow the existing approach. Do you think we should start applying HAS_NO_INCOMPLETE_RANGES now, without waiting for that patch? |
libcxx/include/__ranges/rend.h | ||
87 ↗ | (On Diff #406726) | Thanks for the explanation. Made this function, in both rbegin and rend, follow the "write it 3 times" approach (which also meant replacing the return types). I think the other overloads are fine -- noexcept is exactly the expression being returned, and I think they can get away with returning auto because _LIBCPP_AUTO_CAST is applied to the return type (same as in the existing begin and end). |
LGTM when rebased and CI is happy.
libcxx/include/__ranges/rend.h | ||
---|---|---|
66–67 ↗ | (On Diff #407391) | I think every where else struct __fn is used. |
libcxx/include/__ranges/rend.h | ||
---|---|---|
66–67 ↗ | (On Diff #407391) | Nice catch! Yes please. @var-const there's one other use of class __fn in __ranges/access.h; if you could drive-by change that to struct (perhaps in a separate one-line commit), that'd be appreciated. :) |
87 ↗ | (On Diff #406726) |
Sorry for my vagueness there; I really intended only the noexcept(noexcept(exactly-expression)) part — I wasn't requesting the -> decltype(exactly-expression) part. I think you should remove the -> decltype part, because:
(D115607 is related, in the sense that we might one day choose to consistently use an approach that includes -> decltype, but that's far out of scope for this PR.) |
libcxx/include/__ranges/rbegin.h | ||
---|---|---|
28 ↗ | (On Diff #407044) | I agree. I just spoke with Arthur offline and we agreed on what we needed to do -- we'll try to do it, but the burden shouldn't be on this patch. |
- replace class __fn with struct __fn (in rend.h and access.h);
- make overloads that return std::make_reverse_iterator return auto, not decltype(expr);
- include reverse_iterator.h in rend.h to hopefully fix the modular build.
libcxx/include/__ranges/rend.h | ||
---|---|---|
87 ↗ | (On Diff #406726) | Sure, reverted that part. |
clang-format not found in user’s local PATH; not linting file.