-
Notifications
You must be signed in to change notification settings - Fork 422
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(proto): fix internal representation of OPT #2151
Conversation
Current implementation of OPT represents options as hash map, which prevents same `EdnsCode` from appearing more than once. There are options that may appear more than once (e.g. EDE https://www.rfc-editor.org/rfc/rfc8914.html). Unfortunately, this is a breaking change. I haven't figured out a solution that would not break the API and would not introduce some kind of overhead. BREAKING CHANGE: Changes representation of OPT from `HashMap` to a `Vec`, which means that `as_ref` and `as_mut` also now return `&Vec` rather than `&HashMap`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems reasonable to me -- I don't think breaking the API is a big deal. Thanks!
Maybe you should add a comment explicitly talking about the constraint of some EdnsCode
s being allowed to appear more than once?
That makes sense. I have added the comment as well as a utility for reading all options for a specific code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, would be good to get a review from @bluejekyll.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
Current implementation of OPT represents options as hash map, which prevents same
EdnsCode
from appearing more than once. There are options that may appear more than once(e.g. EDE https://www.rfc-editor.org/rfc/rfc8914.html).
Unfortunately, this is a breaking change. I haven't figured out a solution that would not break the API and would not introduce some kind of overhead.
BREAKING CHANGE: Changes representation of OPT from
HashMap
to aVec
, which means thatas_ref
andas_mut
also now return&Vec
rather than&HashMap
.