<generator>: Mark strengthened noexcept (plus tiny drive-by changes)
#4351
Conversation
stl/inc/generator
Outdated
| @@ -362,7 +362,7 @@ public: | |||
| return *this; | |||
| } | |||
|
|
|||
| _NODISCARD _Ref operator*() const noexcept { | |||
| _NODISCARD _Ref operator*() const noexcept(is_nothrow_copy_constructible_v<_Ref>) { | |||
There was a problem hiding this comment.
This is constructing, but IIRC not necessarily copy constructing, a _Ref. Should this be is_nothrow_constructible_v<_Ref, decltype(*_Coro.promise()._Top.promise()._Ptr)>?
There was a problem hiding this comment.
I think the condition needs to be noexcept(static_cast<_Ref>(*_Coro.promise()._Top.promise()._Ptr)).
If _Ref is an rvalue reference, this is a cast from lvalue to rvalue reference. Such a cast is non-throwing IIUC, but is_nothrow_constructible_v is false.
The standard says noexcept(is_nothrow_copy_constructible_v<reference>), which also needs to be fixed.
There was a problem hiding this comment.
On further thought, I believe the logic here is "If _Ref is a reference type, this function doesn't throw, which is consistent with is_nothrow_copy_constructible_v<_Ref>. If it's not a reference type, it must be a (cv-unqualified?) object type, and _Ptr is a pointer to (const?) _Ref so the static_cast expression in fact does copy a _Ref."
What we've merged here is correct, but we should nonetheless change it in a followup to obviously agree with the Standard wording.
There was a problem hiding this comment.
is_nothrow_copy_constructible_v<_Ref> is correct if:
_Refis an lvalue reference type.- In this case, the function does not throw, and
is_nothrow_copy_constructible_v<_Ref>is true.
- In this case, the function does not throw, and
_Refis an object type.- In this case,
is_nothrow_copy_constructible_v<_Ref>is the right condition.
- In this case,
is_nothrow_copy_constructible_v<_Ref> is not correct if:
_Refis an rvalue reference type.
In this case, the function does not throw, but is_nothrow_copy_constructible_v<_Ref> is false, because,
is_nothrow_copy_constructible_v<_Ref>is equivalent tois_nothrow_constructible_v<_Ref, const _Ref&>.- When
_RefisA&&for some object typeA,const _Ref&isA&. is_constructible_v<A&&, A&>is false, and so isis_nothrow_constructible_v<A&&, A&>.
So I believe that we should not follow the standard wording. We need to fix it.
| @@ -210,7 +210,7 @@ public: | |||
| } | |||
|
|
|||
| _NODISCARD auto yield_value(const remove_reference_t<_Yielded>& _Val) noexcept( | |||
| is_nothrow_constructible_v<remove_cvref_t<_Yielded>, const remove_reference_t<_Yielded>&>) | |||
| is_nothrow_constructible_v<remove_cvref_t<_Yielded>, const remove_reference_t<_Yielded>&>) /* strengthened */ | |||
There was a problem hiding this comment.
Stylistic nitpick: We use /* strengthened */ when it's embedded in a line (especially before an opening brace), but the following requires will always be clang-formatted on a new line, so this should ideally be a C++ comment // strengthened. Not worth resetting testing though.
| _NODISCARD_FRIEND bool operator==(const _Gen_iter& _It, default_sentinel_t) noexcept /* strengthened */ | ||
| { |
There was a problem hiding this comment.
Another stylistic nitpick: There's enough room for the brace here to be attached; clang-format exhibits "sticky" behavior and will preserve it. Also not worth resetting testing for this PR.
StephanTLavavej
merged commit fb6a4cb
into
microsoft:feature/generator
|
Thanks! Had a couple of formatting nitpicks that can be addressed in followup PRs, but the functional changes all look great. 😻 |
Mostly tiny adjustments to better match the standard.
Also implements LWG-3894 "
generator::promise_type::yield_value(ranges::elements_of<Rng, Alloc>)should not benoexcept".Works towards #2936.