Makes UInt an opaque struct type
#568
Conversation
Prior to this patch, `UInt` was an enum offering two storage options: `u64` and `BigUInt`. This patch wraps that enum (now a private type called `UIntData`) in an opaque struct, giving us the ability to change that representation in the future without a breaking change.
| magnitude: UInt { | ||
| data: UIntData::U64(0), | ||
| }, |
There was a problem hiding this comment.
🗺️ magnitude: 064.into() would be more concise, but the trait method into() is not const.
| let mut coefficient_bytes = match &decimal.coefficient.magnitude().data { | ||
| UIntData::U64(unsigned) => unsigned.to_be_bytes().into(), | ||
| UIntData::BigUInt(big) => big.to_bytes_be(), |
There was a problem hiding this comment.
🗺️ This diff contains several instances of this change. Instead of matching directly on the UInt, we now match on its private inner UIntData field.
| @@ -689,7 +690,7 @@ mod tests { | |||
| let mut buffer = BinaryBuffer::new(&[0b1000_0000]); | |||
| let var_int = buffer.read_uint(buffer.remaining())?; | |||
| assert_eq!(var_int.size_in_bytes(), 1); | |||
| assert_eq!(var_int.value(), &UInt::U64(128)); | |||
| assert_eq!(var_int.value(), &UInt::from(128)); | |||
There was a problem hiding this comment.
🗺️ I used TypeName::from instead of value.into() in several places because that's the only way to specify the expected type inline. You can't use turbofish syntax with Into.
| pub struct UInt { | ||
| pub(crate) data: UIntData, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub(crate) enum UIntData { | ||
| U64(u64), | ||
| BigUInt(BigUint), | ||
| } |
There was a problem hiding this comment.
🗺️ This is the core change; the rest of the diff is comprised of ripples.
src/types/integer.rs
Outdated
| )*) | ||
| } | ||
|
|
||
| impl_int_types_from_uint!(i8, i16, i32, i64, i128, u8, u16, u32, u64, u128); |
There was a problem hiding this comment.
🗺️ Previously, we only provided a TryFrom<&UInt> implementation for usize and i64. That was more or less acceptable because UInt wasn't part of the public API. However, I think it needs to be in a couple of places. For example, a Decimal::coefficient method.
| impl TryFrom<&UInt> for i64 { | ||
| type Error = IonError; | ||
|
|
||
| fn try_from(value: &UInt) -> Result<Self, Self::Error> { | ||
| match value { | ||
| UInt::U64(uint) => i64::try_from(*uint).or_else(|_| { | ||
| decoding_error(format!( | ||
| "Unsigned integer {uint:?} was too large to be represented as an i64." | ||
| )) | ||
| }), | ||
| UInt::BigUInt(big_uint) => i64::try_from(big_uint).or_else(|_| { | ||
| decoding_error(format!( | ||
| "Unsigned integer {big_uint:?} was too large to be represented as an i64." | ||
| )) | ||
| }), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl TryFrom<&UInt> for usize { | ||
| type Error = IonError; | ||
|
|
||
| fn try_from(value: &UInt) -> Result<Self, Self::Error> { | ||
| match value { | ||
| UInt::U64(uint) => usize::try_from(*uint).or_else(|_| { | ||
| decoding_error(format!( | ||
| "Unsigned integer {uint:?} was too large to be represented as an usize." | ||
| )) | ||
| }), | ||
| UInt::BigUInt(big_uint) => usize::try_from(big_uint).or_else(|_| { | ||
| decoding_error(format!( | ||
| "Unsigned integer {big_uint:?} was too large to be represented as an usize." | ||
| )) | ||
| }), | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🗺️ These manual implementations of TryFrom have been supplanted by the macro we just saw above.
Prior to this patch,
UIntwas an enum offering two storageoptions:
u64andBigUint.This patch wraps that enum (now a private type called
UIntData) inan opaque struct, giving us the ability to change that representation
in the future without a breaking change.
This also provides a
TryFrom<&UInt>implementation for all primitiveinteger types.
Related to #507.
A follow-on PR will make the same change for the signed
Inttype.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.