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

A feature-flag to change maps representation to BTreeMap #695

Closed
wants to merge 1 commit into from
Closed

A feature-flag to change maps representation to BTreeMap #695

wants to merge 1 commit into from

Conversation

akhramov
Copy link
Contributor

@akhramov akhramov commented Dec 20, 2023

As of now, maps are represented with
std::collections::HashMap, which serves as a robust default. In rarer instances, opting for a different implementation, such as BTreeMap, might be desirable, e.g. for deterministic serialization (#496).

This change

As of now, maps are represented with
[std::collections::HashMap](https://doc.rust-lang.org/stable/std/collections/struct.HashMap.html),
which serves as a robust default. In rarer instances, opting for a
different implementation, such as BTreeMap, might be desirable,
e.g. for deterministic serialization (#496).

This change

* adds `btreemaps` feature flags which changes maps to be backed by
  [std::collections::BTreeMap](https://doc.rust-lang.org/std/collections/struct.BTreeMap.html).
@akhramov akhramov marked this pull request as ready for review December 20, 2023 17:11
@stepancheg
Copy link
Owner

I don't think feature flags should change representation of map.

Because for example, changing adding one dependency to your projection should not make your project much slower because that dependency added protobuf with feature flag.

If deterministic serialization is needed, it could be:

  • an option in serializer to sort keys before serialization
  • codegen option to change field types to BTreeMap (or to IndexMap)

@akhramov
Copy link
Contributor Author

Good points, thanks for your input.

I went with this solution, because some types shipped with the crate would still have HashMap fields, i.e. the generated ones:

pub fields: ::std::collections::HashMap<::std::string::String, Value>,

I guess it is not a dealbreaker for me anyway. I will try to go with the codegen option in another PR.

@akhramov akhramov closed this Dec 21, 2023
@akhramov
Copy link
Contributor Author

Follow up: #696

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.

2 participants