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

Document fields and fix lifetime issue #236

Merged
merged 2 commits into from
Aug 14, 2024
Merged

Document fields and fix lifetime issue #236

merged 2 commits into from
Aug 14, 2024

Conversation

p-avital
Copy link
Contributor

  • Added documentation for fields, allowing bindings to make sense of Bytes
  • Added from_raw_parts constructor.
  • Spotted a lifetime issue which may lead to UB if Bytes is constructed from Arc<&'a [u8]> (for example)

capacity,
vtable: <ptr::NonNull<BytesVt> as From<&'static _>>::from(&ARC_BYTES_VT).cast(),
marker: core::marker::PhantomData,
}
}
}
#[cfg(feature = "alloc")]
impl<'a, T: Sized + AsRef<[u8]> + Send + Sync + 'a> From<Arc<T>> for Bytes<'a> {
impl<'a, T: Sized + AsRef<[u8]> + Send + Sync + 'static> From<Arc<T>> for Bytes<'static> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
impl<'a, T: Sized + AsRef<[u8]> + Send + Sync + 'static> From<Arc<T>> for Bytes<'static> {
impl<T: Sized + AsRef<[u8]> + Send + Sync + 'static> From<Arc<T>> for Bytes<'static> {

Btw, can you expand on the example of Arc<&'not_static [u8]> -> Bytes<'a> being unsound?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this constructor sets a release function in the vtable, Bytes is free to assume it fully owns the data (allowing upgrades without copies), even though an Arc<&'non_static [u8]> doesn't actually own said data.

@p-avital p-avital merged commit 3e1f996 into master Aug 14, 2024
206 checks passed
@p-avital p-avital deleted the bytes-improv branch August 14, 2024 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants