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

Implement AsRef<[u8]> and into_bytes(self) for Unparsed #73

Merged
merged 1 commit into from
May 19, 2024

Conversation

overhacked
Copy link
Collaborator

Add conversion methods to gain access to the inner bytes of Unparsed,
per discussion in #72

Signed-off-by: Ross Williams [email protected]

Add conversion methods to gain access to the inner bytes of `Unparsed`,
per discussion in wiktor-k#72

Signed-off-by: Ross Williams <[email protected]>
@baloo
Copy link
Collaborator

baloo commented May 18, 2024

I'm fine with that, but what I don't get is that it negates the motivation for #67:

This change is motivated by https://github.com/wiktor-k/ssh-agent-lib/pull/66, which--in part--fixes
a bug made possible by pub access to the inner Vec of the Unparsed newtype. Refactoring the
proto::message types into modules ensures that the message parsing implementations can't
directly access the inner fields of the types unless they are deliberately pub. When they were
all in the same module, visibility rules didn't matter.

Or am I missing something?

@overhacked
Copy link
Collaborator Author

it negates the motivation for #67

You're not wrong, but I think that impl AsRef<[u8]> is better than Unparsed(pub Vec<u8>) for a couple of reasons:

  • The logic of encoding & length had been spread across different struct implementations before Modularize proto::messsage #67, and that messy code had been enabled by Unparsed's inner Vec being publicly available. It's just really easy to add .0 to the end of a wrapper type.
  • None of the library code had done it yet, but it's just as easy to take a mutable borrow of the inner Vec, and it being pub meant that library consumers could trample all over the type/API boundaries by treating Unparsed as a Vec by just adding .0 to any instance.
  • AsRef<[u8]>...
    • can never be taken mutably, so that's one improvement.
    • is a known pattern that can be passed to any function that accepts T: AsRef<[u8]>.
    • expresses intent in caller code more clearly (value.as_ref() expresses meaning better than &value.0; the latter requires you to understand the inner structure)
  • Also, the Unparsed.into_bytes() gives you mutable access to the unencoded bytes as needed, but you have to take ownership in order to do so, rather than directly mutate the message.

To me, it feels like better encapsulation, but that may be a preference of flavors. I'm open to suggestions if the API feels not quite right.

@baloo baloo merged commit 874e986 into wiktor-k:main May 19, 2024
16 checks passed
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