-
Notifications
You must be signed in to change notification settings - Fork 212
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 different sized integers #178
Conversation
Great to see you taking care of the project! Cheers |
Thanks ❤️ |
So, I guess this PR should add a bunch of tests to check
|
Hm, so after some more investigation: Our backend crates do not support integers smaller than i64. So I guess there's no point in supporting them either, is there? |
b4f87b7
to
c157bf6
Compare
/// Returns `self` into an i128, if possible. | ||
pub fn into_int128(self) -> Result<i128> { | ||
match self.kind { | ||
ValueKind::I64(value) => Ok(value as i128), |
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.
Consider .into()
instead of casting.
)), | ||
|
||
ValueKind::String(ref s) => { | ||
match s.to_lowercase().as_ref() { |
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.
Why do we do this, but don't transparently allow, say, u128
values that are in range of i128
to convert to i128
?
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 guess you wanted another line to be commented here? I will investigate where we can do this, yes!
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.
No, I meant to ask why we transparently convert strings like yes
or no
to integers, but not integers to other integers. Seemed somewhat counterintuitive.
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.
Ah, now I see what you mean.
This patch tries to mimic the behavior of the non-int128 code... I agree that this might be not what we want for the next release, but I would introduce a change to that behavior in another PR, not in this one.
This broadly LGTM, but I have no say over this project, really. |
Still, your opinion matters to me. Thanks for the review! |
Rebased on master. Still not sure whether I should merge this or not. |
This also enables support for 128 bit integers. Nothing is tested, though. Signed-off-by: Matthias Beyer <[email protected]>
Signed-off-by: Matthias Beyer <[email protected]>
All our backend crates do not support integers smaller than i64, so there's no point in supporting them either. Signed-off-by: Matthias Beyer <[email protected]>
Signed-off-by: Matthias Beyer <[email protected]>
30df243
to
3c1a0ff
Compare
Updated to master, will merge if CI succeeds |
This also enables support for 128 bit integers.
Nothing is tested, though.
I guess this is a draft for now.