Improve panic handling code and communication #569
Conversation
| 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.
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.
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.
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.
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.
And the overhead is alright since it's a rare case to occur anyway (hopefully never)
There was a problem hiding this comment.
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.
Alright, then let's merge as is for now.
No description provided.