Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Dec 17, 2021

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
stl/inc/ranges Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added the ranges C++20/23 ranges label Dec 14, 2021
Copy link
Member

@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.

stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
}
_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
Member

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