Skip to content

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

Merged
LucioFranco merged 1 commit intohyperium:masterfrom
djkoloski:master
Sep 6, 2022
Merged

Remove B: Buf bound on SendStream's parameter#614
LucioFranco merged 1 commit intohyperium:masterfrom
djkoloski:master

Conversation

@djkoloski
Copy link
Contributor

@djkoloski djkoloski commented May 10, 2022

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 djkoloski mentioned this pull request May 10, 2022
Closed
@djkoloski
Copy link
Contributor Author

djkoloski commented May 17, 2022

#616 should fix the nightly CI.

@seanmonstar
Copy link
Member

seanmonstar commented May 19, 2022

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

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

@djkoloski
Copy link
Contributor Author

djkoloski commented May 19, 2022

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

djkoloski commented Jun 7, 2022

Bumping this after #616 was merged.

LucioFranco
LucioFranco approved these changes Jun 22, 2022
View reviewed changes
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

djkoloski commented Aug 24, 2022

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

@LucioFranco
Copy link
Member

LucioFranco commented Sep 6, 2022

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

Reviewers

@LucioFranco LucioFranco LucioFranco approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@djkoloski @seanmonstar @LucioFranco