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

proto: Pass ConnectionId by value internally #2109

Merged
merged 1 commit into from
Dec 23, 2024

Conversation

gretchenfrage
Copy link
Collaborator

@gretchenfrage gretchenfrage commented Dec 21, 2024

Same idea as #2107 but for ConnectionId. ConnectionId is, and probably always will be, Copy. ConnectionId is not remotely large enough to trigger stack overflows or severe performance hazards by being copied. So, we should just simplify the code and let the compiler figure out if it wants to optimize it elsewise.

Plus, this shaves off a few lines by convincing the rustfmt to collapse some multi-line method signatures into one line.

Edit: Changes this PR to only do so internally, and not change the API.

@gretchenfrage gretchenfrage changed the title Pass ConnectionId by value Pass ConnectionId by value Dec 21, 2024
@djc
Copy link
Member

djc commented Dec 21, 2024

I don't think we want to change the public API for this right now.

@gretchenfrage gretchenfrage changed the title Pass ConnectionId by value proto; Pass ConnectionId by value internally Dec 21, 2024
@gretchenfrage
Copy link
Collaborator Author

I don't think we want to change the public API for this right now.

Understandable. I changed this PR so it only changes proto internally, without touching its API. Maybe we just prefer to pass by value in new APIs?

@gretchenfrage gretchenfrage changed the title proto; Pass ConnectionId by value internally proto: Pass ConnectionId by value internally Dec 21, 2024
@Ralith
Copy link
Collaborator

Ralith commented Dec 21, 2024

Feel free to stash the public changes in a separate PR so we don't forget to bring them back in on the next breaking release.

@djc
Copy link
Member

djc commented Dec 23, 2024

Maybe file an issue with a label so that we don't forget to clean up the public API instances of this when we get ready to release 0.12?

@djc djc added this pull request to the merge queue Dec 23, 2024
Merged via the queue into quinn-rs:main with commit 7caa30b Dec 23, 2024
20 checks passed
@gretchenfrage
Copy link
Collaborator Author

Maybe file an issue with a label so that we don't forget to clean up the public API instances of this when we get ready to release 0.12?

@djc to make sure I understand the reasoning: we haven't added semver-breaking changes since the last release, so we currently have the ability for our next release to be a minor version bump, and this change here isn't valuable enough to want to sacrifice that ability, so we want to wait to make such a change until some other PR appears which creates more motivation to do a breaking change. Is my understanding correct?

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