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.

Sign up for GitHub

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

Jump to bottom

<generator>: Mark strengthened noexcept (plus tiny drive-by changes) #4351

Merged
merged 2 commits into from
Jan 31, 2024
Merged

<generator>: Mark strengthened noexcept (plus tiny drive-by changes) #4351

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0e1152c
`<generator>`: Mark strengthened noexcept
cpplearner Jan 30, 2024
c8c7929
Fix conditional noexcept
cpplearner Jan 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions stl/inc/generator
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Copy link
Member

Choose a reason for hiding this comment

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

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.

requires (is_rvalue_reference_v<_Yielded>
&& constructible_from<remove_cvref_t<_Yielded>, const remove_reference_t<_Yielded>&>)
{
Expand All @@ -225,7 +225,7 @@ public:

template <_RANGES input_range _Rng, class _Alloc>
requires convertible_to<_RANGES range_reference_t<_Rng>, _Yielded>
_NODISCARD auto yield_value(_RANGES elements_of<_Rng, _Alloc> _Elem) noexcept {
_NODISCARD auto yield_value(_RANGES elements_of<_Rng, _Alloc> _Elem) {
using _Vty = _RANGES range_value_t<_Rng>;
return _Nested_awaitable<_Yielded, _Vty, _Alloc>{
[](allocator_arg_t, _Alloc, _RANGES iterator_t<_Rng> _It,
Expand All @@ -238,7 +238,7 @@ public:

void await_transform() = delete;

void return_void() noexcept {}
void return_void() const noexcept {}

void unhandled_exception() {
if (_Info) {
Expand Down Expand Up @@ -362,7 +362,7 @@ public:
return *this;
}

_NODISCARD _Ref operator*() const noexcept {
_NODISCARD _Ref operator*() const noexcept(is_nothrow_copy_constructible_v<_Ref>) {
Copy link
Member

Choose a reason for hiding this comment

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

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)>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is_nothrow_copy_constructible_v<_Ref> is correct if:

  • _Ref is an lvalue reference type.
    • In this case, the function does not throw, and is_nothrow_copy_constructible_v<_Ref> is true.
  • _Ref is an object type.
    • In this case, is_nothrow_copy_constructible_v<_Ref> is the right condition.

is_nothrow_copy_constructible_v<_Ref> is not correct if:

  • _Ref is 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 to is_nothrow_constructible_v<_Ref, const _Ref&>.
  • When _Ref is A&& for some object type A, const _Ref& is A&.
  • is_constructible_v<A&&, A&> is false, and so is is_nothrow_constructible_v<A&&, A&>.

So I believe that we should not follow the standard wording. We need to fix it.

_STL_ASSERT(!_Coro.done(), "Can't dereference generator end iterator");
return static_cast<_Ref>(*_Coro.promise()._Top.promise()._Ptr);
}
Expand All @@ -377,8 +377,9 @@ public:
++*this;
}

_NODISCARD bool operator==(default_sentinel_t) const noexcept {
return _Coro.done();
_NODISCARD_FRIEND bool operator==(const _Gen_iter& _It, default_sentinel_t) noexcept /* strengthened */
{
Comment on lines +380 to +381
Copy link
Member

Choose a reason for hiding this comment

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

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.

return _It._Coro.done();
}

private:
Expand Down