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

fix: API IO bounds should use BoundedBuf #217

Merged

Conversation

FrankReh
Copy link
Collaborator

Some of the more recently introduced API IO calls used the original IoBuf and IoBufMut traits without realizing the more general trait is BoundedBuf and BoundedBufMut.

Interesting that callers to the library might not notice until they try to pass a slice of a buffer to one of the IO calls.

Some of the more recently introduced API IO calls used the original
IoBuf and IoBufMut traits without realizing the more general trait is
BoundedBuf and BoundedBufMut.

Interesting that callers to the library might not notice until they try
to pass a slice of a buffer to one of the IO calls.
@FrankReh
Copy link
Collaborator Author

Also interesting that the reviewer(s) didn't catch the misuse of the trait either. (Speaking for a friend.)

@FrankReh
Copy link
Collaborator Author

Asking @mzabaluev and/or @Noah-Kennedy to review. I believe the fixes are straight forward, almost cosmetic.

I didn't change the deref and deref_mut signatures in src/buf/mod.rs. Please confirm that you think those shouldn't be changed either.

@oliverbunting
Copy link

@FrankReh you might consider changing the fixed buffer registries that just got generalised too

@FrankReh FrankReh merged commit 8929e95 into tokio-rs:master Jan 29, 2023
@FrankReh
Copy link
Collaborator Author

you might consider changing the fixed buffer registries that just got generalised too

True that. I wanted to get this in while it was current and I do remember seeing those and wondering but then I forgot. Thanks for the reminder.

@FrankReh FrankReh deleted the frankreh/fix-some-iobuf-signatures branch January 29, 2023 23:20
@FrankReh
Copy link
Collaborator Author

@oliverbunting I'm thinking it's good that the fixed buffer registries are generic over IoBuf.

It is by design that an IoBuf can be passed to an IO operation that takes a BoundedBuf. See Bounded for the implementation. It's also true that a Slice can be passed to the same IO operations.

I guess one way to think of it is the IoBuf and the Slice are specific and the BoundedBuf is general. Maybe there are more precise terms to use. I know we don't have documentation on the main page about this and our design doc is now very misleading.

@mzabaluev
Copy link
Contributor

I didn't see a real need for this change, but if you can pass Vec<Slice<B>> to the vectored operations where you could use Vec, and it does not add any overhead to the original use, why not?

Maybe BoundedBuf could be later changed to a more fundamental abstraction provided by async-io-traits.

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.

4 participants