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
Impl. Integer#size in Rust #212
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.
Looks good. Left some feedback on fatal error handling.
I'd expect this PR to fail for Bignum
since in mruby Fixnum
and Bignum
are not unified like they are in MRI. Filed GH-213 to follow up on Bignum
support.
pub unsafe extern "C" fn size(mrb: *mut sys::mrb_state, slf: sys::mrb_value) -> sys::mrb_value { | ||
let interp = unwrap_interpreter!(mrb); | ||
let nil = Value::convert(&interp, None::<Value>); | ||
if let Ok(_value) = Int::try_convert(&interp, Value::new(&interp, slf)) { |
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 prefer calling is_ok
to check if a Result
is Ok
rather than doing an if let
and ignoring the binding.
if let Ok(size) = Int::try_from(mem::size_of::<Int>()) { | ||
Value::convert(&interp, size).inner() | ||
} else { | ||
nil.inner() |
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 is a fatal error. usize
should always be at least as large as Int
. Typically, we deal with fatal errors by raising a RuntimeError
like
artichoke/artichoke-backend/src/extn/core/matchdata/mod.rs
Lines 146 to 148 in 5d55ce6
Err(captures::Error::Fatal) => { | |
RuntimeError::raise(interp, "fatal MatchData#captures error") | |
} |
nil.inner() | ||
} | ||
} else { | ||
nil.inner() |
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 is a fatal error. slf
should always be convertible to an Int
(or Bignum
) becasue slf
is a Ruby Integer
. Typically, we deal with fatal errors by raising a RuntimeError
like
artichoke/artichoke-backend/src/extn/core/matchdata/mod.rs
Lines 146 to 148 in 5d55ce6
Err(captures::Error::Fatal) => { | |
RuntimeError::raise(interp, "fatal MatchData#captures error") | |
} |
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.
LGTM thanks @tkbky
This PR implements
Integer#size
in Rust.Fixes #179
Ran the tests (integer/size_spec.rb), only passed for the fixnum's not the bignum's.