Skip to content
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

Improve bytes #1174

Merged
merged 23 commits into from
Jun 25, 2024
Merged

Improve bytes #1174

merged 23 commits into from
Jun 25, 2024

Conversation

Mallets
Copy link
Member

@Mallets Mallets commented Jun 19, 2024

  • Refine trait bounds
  • Implement support for HashMap
  • Implement support for char
  • Define explicit Encoding for u8, u16, u32, u64, u128, i8, i16, i32, i64, i128, f32, f64
  • Improve docs

Copy link
Contributor

@evshary evshary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really love the changes 👍

@@ -2345,75 +2925,54 @@ mod tests {
hm.insert(0, 0);
hm.insert(1, 1);
println!("Serialize:\t{:?}", hm);
let p = ZBytes::from_iter(hm.clone().drain());
let p = ZBytes::from(hm.clone());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love it. It's easier to use and understandable now.

}

// Tuple (a, b, c)
macro_rules! impl_tuple3 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the reason we use macro instead of inline here is type issue, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct and trait recursiveness...

}
}

// HashMap
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we can provide something similar for Vec? Now we only support Vec<u8>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately not. Rust does not support serialization and Vec<u8> should have a special treatment compared to Vec<T> where ZSerde: Serialize<T>.
An iterator of T is serialized in this way:

  • Serialize T as standalone
  • Write length of serialized T into final buffer
  • Append serialized T

This allows to iterate over the elements T because we know where the serialized T starts and end.
u8 is a special case of T where we do not want to prepend the serialized length but just copy everything.
But in order to treat it differently, we need specialization in Rust... which is not supported :(

@Mallets Mallets marked this pull request as ready for review June 20, 2024 14:08
@Mallets Mallets marked this pull request as draft June 20, 2024 14:08
@Mallets Mallets marked this pull request as ready for review June 20, 2024 14:37
@Mallets
Copy link
Member Author

Mallets commented Jun 20, 2024

@evshary could you please review this PR?

@Mallets Mallets requested a review from evshary June 20, 2024 14:58
Copy link
Contributor

@evshary evshary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for adding more specific int/uint type in encoding

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants