Implement P2443R1 views::chunk_by #2565
Conversation
stl/inc/ranges
Outdated
|
|
||
| _Iterator() = default; | ||
|
|
||
| _NODISCARD constexpr value_type operator*() const { |
There was a problem hiding this comment.
I wanted to complain about lack of noexcept(is_nothrow_copy_constructible_v<iterator_t<_Vw>>) /* strengthen */
That said it seems we were a bit lazy there with subrange Should we still add it given that we pretty sure know whether it may throw or not?
There was a problem hiding this comment.
This function could also throw if ITERATOR_DEBUG_LEVEL is non-zero and either _Current != _Next throws or _STL_VERIFY calls a user-defined handler that throws. I wasn't sure what to do in this scenario.
That said, <ranges> already has functions that perform similar checks and are noexcept. See also #1269.
stl/inc/ranges
Outdated
| _STL_VERIFY(_Current != _Next, "cannot increment chunk_by_view iterator past end"); | ||
| #endif // _ITERATOR_DEBUG_LEVEL != 0 | ||
| _Current = _Next; | ||
| _Next = _Parent->_Find_next(_Current); |
There was a problem hiding this comment.
This is correct, but gave me a little pause, because I had expected _Next as the argument not the updated _Current.
There was a problem hiding this comment.
This is what P2443R1 says, but I agree with you that _Next feels better.
cpplearner
force-pushed
the
chunk_by
branch
2 times, most recently
from
e87708b to
1dee182
Compare
| noexcept(chunk_by_view(_STD forward<_Rng>(_Range), _STD forward<_Pr>(_Pred)))) requires requires { | ||
| chunk_by_view(_STD forward<_Rng>(_Range), _STD forward<_Pr>(_Pred)); | ||
| } { | ||
| // clang-format on | ||
| return chunk_by_view(_STD forward<_Rng>(_Range), _STD forward<_Pr>(_Pred)); |
There was a problem hiding this comment.
I don't understand why, but I have observed that other views will switch to saying static_cast<T&&>(t) in their requires, despite our usual currently preferred convention of using forward elsewhere. Not sure if this matters here. Example:
Lines 3806 to 3815 in afe0800
There was a problem hiding this comment.
And filter has mixed static_cast and _STD forward.
Lines 1841 to 1844 in afe0800
And transform even uses _STD move.
Lines 2300 to 2303 in afe0800
@CaseyCarter do you remember why this was done in this way?
There was a problem hiding this comment.
We don't replace all uses of forward and move with static_cast due to readability concerns. As a special exception to that rule, we do write static_cast instead of forward and move in concepts and constraints because compiler throughput concerns trump readability - at least until we could complete everything and get real numbers - and we want to avoid instantiating templates that may only ever be used in failing constraint checks.
There was a problem hiding this comment.
Coda: I didn't complain here - despite that this would seemingly violate the general guideline - since there are no competing overloads of "operator() with arity 2" and no views_chunkable concept. I wouldn't mind if you decide to change this for consistency, but I won't demand it either.
cpplearner
force-pushed
the
chunk_by
branch
3 times, most recently
from
915b0b2 to
c6373d2
Compare
|
I've pushed a conflict-free merge with |
|
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
|
Thanks for implementing this chunky chocolate chip cookie! 🍪 🍫 😸 |
This implements P2443R1
views::chunk_by.Fixes #2540.