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 panic-on-drop in quinn_proto::Chunks #1983

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

abonander
Copy link

closes #1703
closes #1531

cc @djc

@abonander
Copy link
Author

I might also recommend making the bool field of ShouldTransmit public to allow destructuring.

@djc
Copy link
Member

djc commented Sep 7, 2024

I might also recommend making the bool field of ShouldTransmit public to allow destructuring.

Do you want to add a commit for this?

@abonander
Copy link
Author

Sure, I just assumed there was a reason it wasn't already public.

@abonander
Copy link
Author

Strictly speaking, it is a potential SemVer hazard if you foresee other fields being added later, e.g. total_bytes_read or something like that.

I don't really care either way. My implementation always checks poll_transmit() if there isn't one pending already.

@djc
Copy link
Member

djc commented Sep 9, 2024

Then why would you recommend making the ShouldTransmit field public?

@abonander
Copy link
Author

I did say "I might recommend", because some people might find it useful.

If ShouldTransmit was only ever intended to be a wrapper for a bool, making the field public makes sense from an API design standpoint.

I was just saying I don't care to die on this hill either way because I don't even use the type. 🤷

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.

quinn-proto: Chunks panic-on-drop behavior is a footgun and makes control flow annoying
2 participants