Skip to content

improve iota_view's iterator's binary operator+ (LWG-3580)#2417

Merged
StephanTLavavej merged 3 commits intomicrosoft:mainfrom
fsb4000:fix2398
Dec 17, 2021
Merged

improve iota_view's iterator's binary operator+ (LWG-3580)#2417
StephanTLavavej merged 3 commits intomicrosoft:mainfrom
fsb4000:fix2398

Conversation

@fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented Dec 14, 2021

Fixes #2398

@fsb4000 fsb4000 requested a review from a team as a code owner December 14, 2021 02:26
@StephanTLavavej StephanTLavavej added the LWG Library Working Group issue label Dec 14, 2021
@StephanTLavavej StephanTLavavej added the ranges C++20/23 ranges label Dec 14, 2021
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already pushed a change to correct the pre-existing issues in my comments.

}
_NODISCARD friend constexpr _Ioterator operator+(const difference_type _Off, _Ioterator _It) noexcept(
noexcept(static_cast<_Wi>(_It._Current + _Off))) /* strengthened */ requires _Advanceable<_Wi> {
is_nothrow_move_constructible_v<_Wi>&& noexcept(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CaseyCarter I observe that this one says is_nothrow_move_constructible_v<_Wi> but the other two said is_nothrow_move_constructible_v<_Ioterator>. You mentioned that they're equivalent, and there seems to be a rationale (as this one constructs an _Ioterator prvalue), but I wanted to point this out in case it was unintentional and should be cleaned up later. No change requested here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other two functions return an lvalue denoting a variable with automatic storage duration, so they are implicitly moved, and that variable is a function argument, so they cannot benefit from NRVO or RVO. Consequently they move construct an _Ioterator.

This function returns a prvalue, so no _Ioterators are moved or copied - C++17 delayed temporary materialization kicks in. It does need to account for potential throws from the _Ioterator constructor from _Wi, however. That constructor is annotated with noexcept(is_nothrow_move_constructible_v<_Wi>. I'm not sure why I decided to "inline" that noexcept-specifier instead of using the more direct is_nothrow_constructible_v<_Ioterator, _Wi>. I really need to work at making my conditional noexcept style more consistent.

@StephanTLavavej StephanTLavavej self-assigned this Dec 16, 2021
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 8578156 into microsoft:main Dec 17, 2021
@StephanTLavavej
Copy link
Member

Thanks for implementing a seventh LWG issue resolution in today's batch! 🚀 🚀 😹

@fsb4000 fsb4000 deleted the fix2398 branch December 17, 2021 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LWG Library Working Group issue ranges C++20/23 ranges

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LWG-3580 iota_view's iterator's binary operator+ should be improved

3 participants