-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add support for TypedValue, a more strongly typed value #96
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.
Hey, thanks for this, sorry for taking a while to get back to you. I have thought about implementing something like this, and so far I've not been convinced that it's that useful, but I'm not committed to that view.
I have not actually checked it, but I have been working with the assumption that a chain of if let Some(s) = Something::from_value(val)
will compile down to the same as a match
. The from_value
functions should all be #[inline]
so the compiler should be able to see and eliminate the duplicate checks of rb_type
.
I think I prefer the chain of if
s as it easily expands nicely to types like Time
, Range
, or Enumerator
which don't have specific basic types.
I'm also unsure the best way to handle RUBY_T_UNDEF
, RUBY_T_IMEMO
, etc.
src/typed_value.rs
Outdated
/// A Ruby Enumerator. | ||
Enumerator(Enumerator), |
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 won't ever match, the Enumerator
class's basic type is RObject
.
src/typed_value.rs
Outdated
/// A Ruby Integer. | ||
Integer(Integer), |
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 this should be separate Fixnum
/Bignum
variants to match the Ruby API.
src/typed_value.rs
Outdated
/// A Ruby Range. | ||
Range(Range), |
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 won't ever match, the Range
class's basic type is RStruct
.
src/typed_value.rs
Outdated
/// A Ruby True. | ||
True, | ||
/// A Ruby False. | ||
False, | ||
/// A Ruby Nil. | ||
Nil, |
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 know they are constants, but I think these should contain Qtrue
, Qfalse
, Qnil
as it would make it API more consistent.
I don't think there's any downside, the size of TypedValue
will be the same regardless, and while containing Qfalse
would prevent niche optimisation (as it is represented as 0
/null
), the Value
variant will already prevent that optimisation (as type wise Value
can be false
).
src/typed_value.rs
Outdated
} | ||
|
||
/// Converts a `TypedValue` into a `Value`. | ||
pub fn as_value(&self) -> Value { |
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.
TypedValue
is Copy
so this can take self
.
why?
often when working with magnus I'd like to have more information about what a
Value
is. When writing code that can accept and handle any value, sometimes I have the need, as an example, to implement a genericget_str_slice
. Now I could just callto_s
on the value, but it incurs performance overhead, as it calls back into ruby and makes a copy to save into a rust string ,even when I the value is an RString, on which I could callas_str
.how?
Introduce a new
TypedValue
which can be constructed from aValue
. It's an enum containing all primitive types, so that developers can dispatch on the enum kind and choose to do different things depending on what theTypedValue
is. I think this is better than a chain of:as I think:
rb_type
only once and we match on it, rather than having a long list of if statementsTypedValue
All in all I think this would be nice to have!
why can't it be done outside of magnus?
all the
from_rb_value_unchecked
arepub(crate)
so building this outside of magnus is impossible.