Skip to content

Commit

Permalink
Enables GATs iteration in the Element API (#442)
Browse files Browse the repository at this point in the history
This PR replaces all uses of `Box<dyn Iterator<Item=_> + 'a>`
in the `Element` and `ElementRef` APIs with generic associated
types. This means that heap allocations are no longer required
to iterate over annotations, sequence elements, or struct fields.

Because this change uses generic associated types, it also
configures the CI tests to use the beta channel of cargo. GATs will
be available in the next release of Rust (v1.65, out in November);
once that happens, we can and should switch back to 
stable. (#443)

This change also removes the recently added dependency
on the `linkhash` crate. That dependency was originally pulled
in so structs could remember the order in which their fields were
inserted for later iteration. However, it only worked if the struct
did not contain duplicate field names. This PR changes the field
storage for `Struct` and `StructRef` to store their fields in a
`Vec` and also maintain a `HashMap<Symbol, IndexVec>`
that remembers all of the indexes at which values for a given
field can be found.

Further, it moves `Sequence` elements and `Struct` fields into
an `Rc` to make clone() operations cheaper. A future PR will
modify iterators over collection types to clone the `Rc` and
thus remove one of their lifetime constraints, making it much
easier to write a recursive iterator over an element tree without
constant cloning.
  • Loading branch information
zslayton committed Oct 13, 2022
1 parent 1ab131a commit 9a6196e
Show file tree
Hide file tree
Showing 15 changed files with 531 additions and 222 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/coverage.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ jobs:
with:
profile: minimal
# nightly can be very volatile--pin this to a version we know works well...
toolchain: nightly-2022-05-01
toolchain: nightly-2022-10-12
override: true
- name: Cargo Test
uses: actions-rs/cargo@v1
Expand Down
6 changes: 5 additions & 1 deletion .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,11 @@ jobs:
uses: actions-rs/toolchain@v1
with:
profile: minimal
toolchain: stable
# We're temporarily testing against `beta` to have access to GATs. When Rust v1.65 is released in
# 2022-11, we should change this back to `stable`.
# See: https://github.com/amzn/ion-rust/issues/443
toolchain: beta
components: rustfmt, clippy
override: true
- name: Cargo Build
uses: actions-rs/cargo@v1
Expand Down
1 change: 0 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ num-bigint = "0.3"
num-integer = "0.1.44"
num-traits = "0.2"
arrayvec = "0.7"
hashlink = "0.8.1"
smallvec = "1.9.0"

[dev-dependencies]
Expand Down
6 changes: 3 additions & 3 deletions examples/read_all_values.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ fn read_all_values<R: RawReader>(reader: &mut R) -> IonResult<usize> {
match ion_type {
Struct | List | SExpression => reader.step_in()?,
String => {
let _text = reader.map_string(|_s| ())?;
reader.map_string(|_s| ())?;
}
Symbol => {
let _symbol_id = reader.read_symbol()?;
Expand All @@ -83,10 +83,10 @@ fn read_all_values<R: RawReader>(reader: &mut R) -> IonResult<usize> {
let _boolean = reader.read_bool()?;
}
Blob => {
let _blob = reader.map_blob(|_b| ())?;
reader.map_blob(|_b| ())?;
}
Clob => {
let _clob = reader.map_clob(|_c| ())?;
reader.map_clob(|_c| ())?;
}
Null => {}
}
Expand Down
2 changes: 1 addition & 1 deletion src/binary/int.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ impl DecodedInt {
let empty_leading_bytes: u32 = (magnitude.leading_zeros() - 1) >> 3;
let first_occupied_byte = empty_leading_bytes as usize;

let mut magnitude_bytes: [u8; mem::size_of::<u64>()] = (magnitude as u64).to_be_bytes();
let mut magnitude_bytes: [u8; mem::size_of::<u64>()] = magnitude.to_be_bytes();
let bytes_to_write: &mut [u8] = &mut magnitude_bytes[first_occupied_byte..];
if value < 0 {
bytes_to_write[0] |= 0b1000_0000;
Expand Down
4 changes: 2 additions & 2 deletions src/binary/non_blocking/raw_binary_reader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ struct EncodedValue {
impl EncodedValue {
/// Returns the offset of the current value's type descriptor byte.
fn header_offset(&self) -> usize {
self.header_offset as usize
self.header_offset
}

/// Returns the length of this value's header, including the type descriptor byte and any
Expand Down Expand Up @@ -734,7 +734,7 @@ impl<A: AsRef<[u8]>> IonReader for RawBinaryBufferReader<A> {
let coefficient_size_in_bytes =
encoded_value.value_length() - exponent_var_int.size_in_bytes();

let exponent = exponent_var_int.value() as i64;
let exponent = exponent_var_int.value();
let coefficient = buffer.read_int(coefficient_size_in_bytes)?;

if coefficient.is_negative_zero() {
Expand Down
6 changes: 3 additions & 3 deletions src/binary/raw_binary_reader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ struct EncodedValue {
impl EncodedValue {
/// Returns the offset of the current value's type descriptor byte.
fn header_offset(&self) -> usize {
self.header_offset as usize
self.header_offset
}

/// Returns the length of this value's header, including the type descriptor byte and any
Expand Down Expand Up @@ -526,7 +526,7 @@ impl<R: IonDataSource> IonReader for RawBinaryReader<R> {
let coefficient_size_in_bytes =
self.cursor.value.value_length - exponent_var_int.size_in_bytes();

let exponent = exponent_var_int.value() as i64;
let exponent = exponent_var_int.value();
let coefficient = self.read_int(coefficient_size_in_bytes)?;

if coefficient.is_negative_zero() {
Expand Down Expand Up @@ -938,7 +938,7 @@ where
#[inline(always)]
fn read_var_int(&mut self) -> IonResult<VarInt> {
let var_int = VarInt::read(&mut self.data_source)?;
self.cursor.bytes_read += var_int.size_in_bytes() as usize;
self.cursor.bytes_read += var_int.size_in_bytes();
Ok(var_int)
}

Expand Down
2 changes: 1 addition & 1 deletion src/binary/var_uint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ impl VarUInt {
/// unsigned integer
#[inline(always)]
pub fn size_in_bytes(&self) -> usize {
self.size_in_bytes as usize
self.size_in_bytes
}
}

Expand Down
15 changes: 15 additions & 0 deletions src/symbol_ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,21 @@ impl AsSymbolRef for Symbol {
}
}

impl AsSymbolRef for &Symbol {
fn as_symbol_ref(&self) -> SymbolRef {
self.text()
.map(SymbolRef::with_text)
.unwrap_or_else(SymbolRef::with_unknown_text)
}
}

impl<'borrow, 'data> AsSymbolRef for &'borrow SymbolRef<'data> {
fn as_symbol_ref(&self) -> SymbolRef<'data> {
// This is essentially free; the only data inside is an Option<&str>
(*self).clone()
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down
Loading

0 comments on commit 9a6196e

Please sign in to comment.