Implement views::transform #1258
Conversation
CaseyCarter
added
cxx20
| if constexpr (_Offset_verifiable_v<iterator_t<_Base>>) { | ||
| _Current._Verify_offset(_Off); | ||
| } else { | ||
| if (_Off < 0) { | ||
| if constexpr (sized_sentinel_for<iterator_t<_Base>, iterator_t<_Base>>) { | ||
| _STL_VERIFY(_Off >= _RANGES begin(_Parent->_Range) - _Current, | ||
| "cannot seek transform_view iterator before begin"); | ||
| } | ||
| } else if (_Off > 0) { | ||
| if constexpr (sized_sentinel_for<sentinel_t<_Base>, iterator_t<_Base>>) { | ||
| _STL_VERIFY(_Off <= _RANGES end(_Parent->_Range) - _Current, | ||
| "cannot seek transform_view iterator after end"); | ||
| } else if constexpr (sized_sentinel_for<iterator_t<_Base>, | ||
| iterator_t<_Base>> && sized_range<_Base>) { | ||
| const auto _Size = _RANGES distance(_Parent->_Range); | ||
| _STL_VERIFY(_Off <= _Size - (_Current - _RANGES begin(_Parent->_Range)), | ||
| "cannot seek transform_view iterator after end"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I feel like this could be a usefull helper function if passed a error message
There was a problem hiding this comment.
I'd like to put off refactoring this for future work. I think there will be more places we can use it, but I hate to make a separate function before we need it. Doubly so given that we kind of need to merge this tomorrow.
StephanTLavavej
left a comment
StephanTLavavej
left a comment
There was a problem hiding this comment.
Approved with questions.
| #if _ITERATOR_DEBUG_LEVEL == 0 | ||
| #define _NOEXCEPT_IDL0(...) noexcept(__VA_ARGS__) | ||
| #else | ||
| #define _NOEXCEPT_IDL0(...) | ||
| #endif // _ITERATOR_DEBUG_LEVEL == 0 |
There was a problem hiding this comment.
I don't like the idea of noexcept being dependent on debug level. If debug machinery throws the program should just terminate. It also makes the code harder to understand.
There was a problem hiding this comment.
I'm also surprised to see this at the last minute. In vector and string we've allowed debug allocations to slam into noexcept (which causes headaches occasionally, but there are no good choices there).
There was a problem hiding this comment.
I could live with "just terminate" - it certainly was simpler - but I expect we'll hear complaints about it. I reserve the right to say "I told you so!" when that day comes ;)
There was a problem hiding this comment.
After thinking about it a little more, termination does seem problematic - we should probably err on the side of not strengthening noexcept, or making the precondition checks controlled by the noexceptness of the operations they want to invoke. @CaseyCarter will file a tracking issue to investigate this (due to time constraints we had to merge this for 16.8 Preview 3 as-is). Thanks for finding this, @AdamBucior!
There was a problem hiding this comment.
Stephan and I went around in circles a bit about how to resolve this and decided to merge as-is (since this is already merged internally) and file an issue to cleanup this conditional noexcept in a followup change. I'll file that issue after this merges - I'd like to be able to link to the significant bits of code - and post the issue number here to close the loop.
5 participants
Partially addresses #39. This is the last of the pieces we're planning to merge Friday.