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

Remove B: Buf bound on SendStream's parameter #614

Merged
merged 1 commit into from
Sep 6, 2022

Conversation

djkoloski
Copy link
Contributor

This bound is only necessary when calling methods on SendStream because we don't use any associated types of B's Buf impl in the type. Relaxing this bound allows us to construct SendStreams with non-Buf B (though we can't do anything with them).

@djkoloski
Copy link
Contributor Author

#616 should fix the nightly CI.

@seanmonstar
Copy link
Member

(though we can't do anything with them).

Is this beneficial somehow? I'd love to understand the goal.

@djkoloski
Copy link
Contributor Author

Sorry, I didn't put enough context in here. hyperium/hyper#2830 contains some work to remove a transmute from hyper. Rather than write new Buf impls with unreachable!, I figured we could clean things up bit by relaxing the bounds in h2. That way we don't have to write uncallable Buf impls with impossible methods. It's not very important, but it does help with code quality downstream.

@djkoloski
Copy link
Contributor Author

Bumping this after #616 was merged.

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

This seems fine to me, I prefer to not have the bounds on the struct anyways. Also, I don't think this is breaking so I think we are fine to merge?

@djkoloski
Copy link
Contributor Author

Quick bump on this, if there are no objections I think this would be a good improvement. 🙂

@LucioFranco
Copy link
Member

Oh sorry, we can get this merged :D

@LucioFranco LucioFranco merged commit 756e252 into hyperium:master Sep 6, 2022
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