- std::ranges::begin
- std::ranges::cbegin
- std::ranges::end
- std::ranges::cend
- std::ranges::iterator (required for end)
Implements parts of:
- P0896R4 The One Ranges Proposal`
Co-author: @zoecarver
Differential D100255
[libcxx][ranges] adds `range` access CPOs cjdb on Apr 10 2021, 10:36 PM. Authored by
Details
Implements parts of:
Co-author: @zoecarver
Diff Detail
Unit Tests Event TimelineThere are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions I'll have a close look once it passes the build server. I expect pre-merge errors due to conflicts with D90999. Both modify add the <ranges> header.
Comment Actions Not yet finished reviewing, but I've been accumulating comments here over the past few days and want to submit them before stuff moves around too much. I'll have more comments later when I do a proper review.
Comment Actions The array-of-incomplete-element case is IFNDR. I think we really want that case to be a SFINAE-unfriendly hard error, since it's a recipe for disaster (ODR violations). Comment Actions
Comment Actions Okay, to be honest, I really do not like these tests. I think we need to workshop the structure here because the current tests fall down in terms of readability and demonstrability. We can get to correctness and completeness later, but let's get the model right first. Tests shouldn't have too much boilerplate, and in the test file itself, it should be clear what is being tested and how it's being tested. There should be enough context to figure out what's going on. The current tests in ranges/range.access/* fail at this. There is a lot of boilerplate. We should try to be modest with the use of macros and only use them when they are unequivocally the best answer, and there isn't another clear way to do something. In these tests everything, including the main function, is hidden inside of a macro. This is not readable. A good test would look like this: struct a_clearly_named_type; static_assert(!is_invocable_v<RangesBeginT, a_clearly_named_type>); There are a few things here: first, it is clear what is being tested because of the context. The name CHECK_MEMBER_ACCESS tells me very little. However, the expression !is_invocable_v tells me that we shouldn't be able to call ranges::begin because of some constraint. Hopefully, the type a_clearly_named_type explains what constraint is being violated. Second, it's easily verifiable. If I'm reading this test with the standard next to me, I can easily match the text in the standard to the static_assert and check it off mentally. It is much more difficult to do this when you have to follow the input into a macro full of boilerplate and distractions where the code is generated. My mind does not have a large enough cache for this to be feasible, sorry :P It would be one thing if these macros were stress testing concepts, or ranges, or whatever. But they're not. They're being used for the tests that are supposed to demonstrate the API and its correctness. The issues above are making it hard for me to verify correctness, but these tests are definitely not complete. For example, where are the tests for a type without any members? Or a member with the wrong return type? I think there are a few tests you need to add; luckily, I think these will be immediately obvious with a new test structure. Comment Actions I agree, will look into this.
Comment Actions I also agree with Zoe on the point of testing, but it's been said before so no need for me to. I'm looking forward to the update!
Comment Actions When it gets implemented. We're working towards that. I think Zoe was working on it this week. Also, this is the wrong place to ask for this. This is the right forum for code reviews. Please take a look at our Discord channel for other questions.
Comment Actions
Comment Actions @cjdb the following should work (but doesn't): static_assert(std::is_invocable_v<RangeBeginT, int (&)[]>); Comment Actions A few nits but other than that this LGTM. Let's finally get this landed (as soon as Louis signs off on it)!
Comment Actions Let's land this!
Comment Actions Most of Zoe's comments have been applied just before merging with main.
|
I have to admit this is one instance of a split that, IMO, doesn't add anything. I think it makes a lot more sense to group those into the same header since they are basically always used together (you pretty much always need end when you use begin and vice versa).
I'd throw that into something like __ranges/access.h.