Skip to content

Add bytes to type reference helper methods to FromBytes - #526

Merged
joshlf merged 1 commit into
google:mainfrom
kupiakos:ref-helper
Oct 27, 2023
Merged

Add bytes to type reference helper methods to FromBytes#526
joshlf merged 1 commit into
google:mainfrom
kupiakos:ref-helper

Conversation

@kupiakos

Copy link
Copy Markdown
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

joshlf commented Oct 19, 2023

Copy link
Copy Markdown
Member

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

kupiakos commented Oct 19, 2023

Copy link
Copy Markdown
Contributor Author

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

joshlf commented Oct 19, 2023

Copy link
Copy Markdown
Member

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.

@joshlf joshlf left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/lib.rs Outdated
Comment thread src/lib.rs Outdated
Comment thread src/lib.rs
@kupiakos

Copy link
Copy Markdown
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`.

@joshlf joshlf left a comment

Copy link
Copy Markdown
Member

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
@joshlf

joshlf commented Oct 27, 2023

Copy link
Copy Markdown
Member

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.

3 participants