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

Add bytes to type reference helper methods to FromBytes #526

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

kupiakos
Copy link
Contributor

One common complaint I receive is how onerous it is to convert from bytes to type references in zerocopy for simple cases. I fully agree; we don't use the Ref type in most of our code. AsBytes::as_bytes makes it easy to go from &T to &[u8], so why is there such a dance to go the other way?

These helper methods intend to improve the developer experience. They follow the same naming and error handling scheme as read_from. I would rather these stay named as such until a refactor of read_from and write_to occurs to match - I find that consistency valuable.

These avoid a tuple return in from_[suffix|prefix] - again for developer UX since we usually threw these out.

    fn ref_from(bytes: &[u8]) -> Option<&Self> where Self: Sized;
    fn ref_from_prefix(bytes: &[u8]) -> Option<&Self> where Self: Sized;
    fn ref_from_suffix(bytes: &[u8]) -> Option<&Self> where Self: Sized;

    fn mut_from(bytes: &mut [u8]) -> Option<&mut Self> where Self: Sized + AsBytes;
    fn mut_from_prefix(bytes: &mut [u8]) -> Option<&mut Self> where Self: Sized + AsBytes;
    fn mut_from_suffix(bytes: &mut [u8]) -> Option<&mut Self> where Self: Sized + AsBytes;

@kupiakos kupiakos requested a review from joshlf October 19, 2023 19:14
@joshlf
Copy link
Member

joshlf commented Oct 19, 2023

Awesome, thanks for this! Taking a look now...

Do y'all actually make use of from_[suffix|prefix]? I'm curious what your use cases are since I generally have found that those APIs have very few users.

@kupiakos
Copy link
Contributor Author

kupiakos commented Oct 19, 2023

Do y'all actually make use of from_[suffix|prefix]?

We don't use suffix like ever, but we do use prefix. We rarely check the exact size, just that it's large enough.
I'm supportive of removing _suffix, but I do like it there for consistency.

@joshlf
Copy link
Member

joshlf commented Oct 19, 2023

Do y'all actually make use of from_[suffix|prefix]?

We don't use suffix like ever, but we do use prefix. We rarely check the exact size, just that it's large enough. I'm supportive of removing _suffix, but I do like it there for consistency.

No objection to supporting both; I'm just curious what your use cases are.

Copy link
Member

@joshlf joshlf left a comment

Choose a reason for hiding this comment

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

Looks good! A few small nits.

I'm going to hold off on approving until we resolve the discussion in #524, and I also want to wait until some other folks have a chance to chime in who aren't working this week. I expect we can have this all resolved and merged early next week.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@kupiakos
Copy link
Contributor Author

No objection to supporting both; I'm just curious what your use cases are.

There are probably valid use cases for _from_suffix, maybe for dynamic data, but it's just never come up for us from what I've seen.

It's common for a subsystem like TPM to receive a buffer that is larger than the message it contains when the request buffer is shared for the response. _from_prefix is particularly useful there.

These helper methods intend to improve the developer experience.
They follow the same naming and error handling scheme as `read_from`.
Copy link
Member

@joshlf joshlf left a comment

Choose a reason for hiding this comment

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

We had another request for this, so I'm going to land it and follow up on the outstanding comments in a follow-up PR.

@joshlf joshlf added this pull request to the merge queue Oct 27, 2023
Merged via the queue into google:main with commit 5eeb474 Oct 27, 2023
126 checks passed
@joshlf
Copy link
Member

joshlf commented Oct 27, 2023

This has been published in 0.7.18.

kupiakos added a commit to kupiakos/zerocopy that referenced this pull request Oct 27, 2023
@kupiakos kupiakos mentioned this pull request Oct 27, 2023
kupiakos added a commit to kupiakos/zerocopy that referenced this pull request Oct 27, 2023
@kupiakos kupiakos deleted the ref-helper branch October 27, 2023 21:02
github-merge-queue bot pushed a commit that referenced this pull request Oct 27, 2023
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.

None yet

3 participants