-
Notifications
You must be signed in to change notification settings - Fork 12
Print detailed panic messages #24
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
Print detailed panic messages #24
Conversation
eholk
left a comment
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.
This seems like a nice ergonomic improvement. Thank you!
It looks like you need to sign the CLA, but I'd be happy to merge this patch once we're all good on that part.
src/lib.rs
Outdated
| if !StackFuture::<F, STACK_SIZE>::has_space_for::<F>() { | ||
| panic!("F is too large"); | ||
| concat_panic!( | ||
| "F is too large: ", |
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.
| "F is too large: ", | |
| "Future is too large: ", |
I feel like seeing Future instead of F will make it clearer to the user what is wrong. Maybe "Future F is too large" would be even clearer...
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.
Good idea. I changed the panic messages according to your suggestion and also suppressed the 'dead_code' linter errors in the tests.
src/lib.rs
Outdated
| if !StackFuture::<F, STACK_SIZE>::has_alignment_for::<F>() { | ||
| panic!("F has incompatible alignment"); | ||
| concat_panic!( | ||
| "F has incompatible alignment: ", |
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.
| "F has incompatible alignment: ", | |
| "Future has incompatible alignment: ", |
|
@microsoft-github-policy-service agree |
|
It looks like your CI is failing due to existing issues. I'll fix these over in #25. |
|
Ah, unfortunately, we both added the |
5029c4b to
611c6af
Compare
|
@eholk sorry for the late reply; I rebased the branch and dropped the last commit, should be fine now. |
|
Awesome, thanks! I just merged it. |
This PR adds support for printing detailed error messages when there is not enough space or an alignment mismatch.