-
Notifications
You must be signed in to change notification settings - Fork 10
Update documentation and add StatusAnd::unwrap()
#15
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
Most types didn't have any documentation. Add it.
24d2c5d
to
4ac3904
Compare
4ac3904
to
bd3af16
Compare
This provides an easy way to get the value and check for errors.
bd3af16
to
a3dea6e
Compare
@fmease are you able to merge this since you've at least given a read through? I'd love to at least get the docs in |
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.
Thanks!
@@ -90,6 +90,14 @@ impl<T> StatusAnd<T> { | |||
} | |||
} | |||
|
|||
impl<T: core::fmt::Debug> StatusAnd<T> { | |||
/// Extract the inner value if there were no errors. If there were errors, panic. | |||
pub fn unwrap(self) -> T { |
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 can't really judge the usefulness(*) of this but I guess it's fine to include in the public API.
(*): I don't know the consumer base of this crate.
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 think the main use case of this crate is testing softfloat libraries, e.g. compiler_builtins and libm. This method is just a convenience because the vast majority of the time you don't expect NaN and just want the inner value, but NaN would be an error so you should at least detect it.
Thanks for the review! Could you do a patch release too? (Looks like it's just updating Cargo.toml then creating a GH release? I can do the Cargo.toml PR if you'd like) |
I don't have the rights to do so (as a member of compiler-contributors (which has only been merged into compiler on paper)) but I will ping a relevant member once we've updated the package manifest.
That would be awesome! I would merge it straightaway. |
This
unwrap()
function gives an easy way to get the value while checking there were no errors.We were missing documentation for almost everything so I added it as the first commit of this PR.
cc @eddyb since some of your fixmes got fixed