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

explicitly document finally actions must be non-throwing #1190

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

explicitly document finally actions must be non-throwing #1190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8eb3205
explicitly document finally actions must be non-throwing
apenn-msft Feb 3, 2025
5a66c65
update comments in source
apenn-msft Feb 3, 2025
5f0ff1d
Update README.md
apenn-msft Feb 3, 2025
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
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ dyn_array | &#x26
[**4. Utilities**][cg-utilities] | |
move_owner | ☐ | A helper function that moves one `owner` to the other
[byte](docs/headers.md#user-content-H-byte-byte) | ☑ | Either an alias to `std::byte` or a byte type
[final_action](docs/headers.md#user-content-H-util-final_action) | ☑ | A RAII style class that invokes a functor on its destruction
[final_action](docs/headers.md#user-content-H-util-final_action) | ☑ | A RAII style class that invokes a non-throwing functor on its destruction
[finally](docs/headers.md#user-content-H-util-finally) | ☑ | A helper function instantiating [final_action](docs/headers.md#user-content-H-util-final_action)
[GSL_SUPPRESS](docs/headers.md#user-content-H-assert-gsl_suppress) | ☑ | A macro that takes an argument and turns it into `[[gsl::suppress(x)]]` or `[[gsl::suppress("x")]]`
[[implicit]] | ☐ | A "marker" to put on single-argument constructors to explicitly make them non-explicit
Expand Down
6 changes: 3 additions & 3 deletions docs/headers.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -794,7 +794,7 @@ template <class F>
class final_action { ... };
```

`final_action` allows you to ensure something gets run at the end of a scope.
`final_action` allows you to ensure non-throwing code is executed at the end of a scope.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not convinced this is correct. It suggests that throwing code will not be executed, but that isn't the case. When code that throws is executed, the program will call std::terminate().

Copy link
Author

Choose a reason for hiding this comment

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

yep I also struggled to convey this. so hoping we can let the compiler enforce pending #1193 so we don't need tricky language. basically we want to document that while you could pass throwing code here the utility (at least as currently written) only really works as expected with non-throwing code.

Copy link
Author

Choose a reason for hiding this comment

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

it seems scope_exit proposal documents this behavior on the constructor as:
https://en.cppreference.com/w/cpp/experimental/scope_fail/scope_fail

 The behavior is undefined if calling fn() throws an exception or results in undefined behavior, even if fn has not been called.


See [E.19: Use a final_action object to express cleanup if no suitable resource handle is available](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Re-finally)

Expand All @@ -805,13 +805,13 @@ explicit final_action(const F& ff) noexcept;
explicit final_action(F&& ff) noexcept;
```

Construct an object with the action to invoke in the destructor.
Construct an object with the non-throwing action to invoke in the destructor.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto. throwing actions will still be invoked.

Copy link
Author

Choose a reason for hiding this comment

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

yep, will circle back pending discussion of #1193 since if we can enforce this via concepts/requires then we can say in code the action must be noexcept and we need not document it


```cpp
~final_action() noexcept;
```

The destructor will call the action that was passed in the constructor.
The destructor will invoke the action that was passed in the constructor; if the action throws an exception the program will terminate.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is the necessary documentation change for now, but maybe we need to say "throws an uncaught exception". The following is OK:

gsl::finally([] {
  try {
    throw 1;
  } catch (...) {}
);

Copy link
Author

Choose a reason for hiding this comment

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

that is explicit yep although thrown seems to common language the standard uses to mean an exception leaves a function, e.g.
N4928 14.3 452

 If an exception is thrown during the destruction of temporaries or local variables for a return statement (8.7.4), the destructor for the returned object (if any) is also invoked.

I guess it's a nuance of throws vs thrown vs thrown from. Let me try

if an exception is thrown from the action, the program will terminate

Copy link
Author

Choose a reason for hiding this comment

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

although we also see 'throws' used in this capacity as well
https://en.cppreference.com/w/cpp/experimental/scope_fail/scope_fail

The behavior is undefined if calling fn() throws an exception or results in undefined behavior, even if fn has not been called.


```cpp
final_action(final_action&& other) noexcept;
Expand Down
2 changes: 1 addition & 1 deletion include/gsl/util
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ namespace gsl
// index type for all container indexes/subscripts/sizes
using index = std::ptrdiff_t;

// final_action allows you to ensure something gets run at the end of a scope
// final_action allows you to ensure non-throwing code is executed at the end of a scope.
template <class F>
class final_action
{
Expand Down