-
Notifications
You must be signed in to change notification settings - Fork 102
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
Improve panic handling code and communication #569
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
fn handle_vm_panic_works() { | ||
fn nice_try() { | ||
panic!("oh no!"); | ||
} | ||
let err = catch_unwind(nice_try).unwrap_err(); | ||
handle_vm_panic("nice_try", err); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a bit of a nitpick, but I'm not sure how I feel about this test. It's really testing catch_unwind
rather than handle_vm_panic
. Basically the only thing this ensures about our code is that handle_vm_panic
doesn't panic itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree for the automated case and it might not be a text book style test. However, it is very handy for manual inspection of the console output (which revealed the Any
debug problem) as well as ensuring the function can be used in isolation with the weird input type. Do you have an idea how to improve the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The guard pattern maybe? A bit verbose but no parameter needed?
static VM_WRITER: RefCell<Option<Box<dyn Writer>>> = RefCell::new(None);
fn with_writer<W: Write + 'static, F: Fn(&dyn Writer)>(w: W, func: F) {
let prev = VM_WRITER.get();
VM_WRITER.set(Some(Box::new(w)));
access_writer(func);
VM_WRITER.set(prev);
}
fn access_writer<F: Fn(&dyn Writer)>(func: F) {
VM_WRITER.with_borrow(func);
}
[...]
access_writer(|w| writeln!(w, "hello world!"));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that code itself won't work immediately because .with_borrow
isn't enough and it needs a fallback case to io::stdout()
but close enough to illustrate what I mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the overhead is alright since it's a rare case to occur anyway (hopefully never)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong opinion on if/how to improve this. But if there are no easy wins, I'd merge this as is and leave it up to you to improve it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, then let's merge as is for now.
No description provided.