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

Adding catch_panic anti-pattern #22

Closed
wants to merge 2 commits into from

Conversation

liamzdenek
Copy link

No description provided.

@@ -48,7 +48,7 @@ language.

### Anti-patterns

* TODO thread + catch_panic for exceptions
* [thread + catch_panic for exceptions](anti_patterns/catch_panic.md)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you change the title here to match the title in the description

Copy link
Author

Choose a reason for hiding this comment

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

fixed in dc01184

@nrc
Copy link
Collaborator

nrc commented Sep 21, 2016

@liamzdenek huge apologies for letting this and your other PR slip off my radar - it got pushed down my notifications and I forgot about it. I'll get some feedback for you before the end of the week.

cc @cbreeden, @lfairy if you have time to comment here it would be appreciated.

README.md Show resolved Hide resolved

// the program continues running despite the panic
println!("potential error: {:?}", result);
assert!(result.is_err());
Copy link
Collaborator

Choose a reason for hiding this comment

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

The example could be improved by adding a function and which panics and catching the panic in the caller, then matching the Result. Describing the example you could show how by returning a Result, the Result-ness of the function is described in the signature.


Expected errors should not result in stack unwinding. Instead, expected errors
should be handled through the Result and Option types. [The Rust Book's chapter
on Error Handling](https://doc.rust-lang.org/book/error-handling.html) elaborates further on this.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add why stack unwinding is bad? And perhaps the other disadvantages of using catch_unwind? Also, where is it appropriate to use catch_unwind

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also: since Result::unwrap() converts the error to a string, it's harder to distinguish between different kinds of errors than if we had matched the result directly.

@pickfire
Copy link
Contributor

Ping, @liamzdenek are you still alive?

@simonsan simonsan added the C-addition Category: Adding new content, something that didn't exist in the repository before label Dec 31, 2020
@simonsan
Copy link
Collaborator

@liamzdenek any updates on this?

@simonsan simonsan added the C-waiting for Category: Waiting for feedback of the initial author or some external dependency/issue label Dec 31, 2020
@simonsan
Copy link
Collaborator

simonsan commented Jan 1, 2021

Closing due to inactivity. Further additions to the PR can be made in #109.

@simonsan simonsan closed this Jan 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-addition Category: Adding new content, something that didn't exist in the repository before C-waiting for Category: Waiting for feedback of the initial author or some external dependency/issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants