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

explicitly document finally actions must be non-throwing #1190

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

Conversation

apenn-msft
Copy link

because the gsl::final_action destructor is marked noexcept(true), the action cannot throw else the program will terminate; this nuance should be documented explicitly and (to be investigated later) ideally enforced in code.

because the gsl::final_action destructor is marked noexcept(true), the action cannot throw else the program will terminate; this nuance should be documented explicitly and (to be investigated later) ideally enforced in code.
@apenn-msft
Copy link
Author

@apenn-msft please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree company="Microsoft"

@apenn-msft
Copy link
Author

Note I have companion PRs here:
#1192
#1191

This PR is a base of
#1191
(upgrades documentation to code enforcement)
but an alternative to
#1192
(avoids unnecessary crashes at runtime)

The different PRs are meant to gauge which the community best likes. In all 3 PRs the same problem we want to address is that gsl finally makes it too easy to write code that would cause surprise crashes at runtime.

Copy link
Collaborator

@carsonRadtke carsonRadtke left a comment

Choose a reason for hiding this comment

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

Updating the documentation to teach people that uncaught exceptions in the final action causes std::terminate() sounds like a good change.

I think we want to be clear that what happens in the action is arbitrary, but if an uncaught exceptions leaks, then the program will be stopped.


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

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

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

@carsonRadtke carsonRadtke added the Type: Documentation Related to documentation updates label Feb 5, 2025
@apenn-msft
Copy link
Author

pending discussion of #1193

@apenn-msft apenn-msft marked this pull request as draft February 6, 2025 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Documentation Related to documentation updates
Projects
Status: Changes Requested
Development

Successfully merging this pull request may close these issues.

2 participants