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

Impl Databake for maps/sets #4266

Closed
adamchalmers opened this issue Nov 9, 2023 · 5 comments · Fixed by #4295
Closed

Impl Databake for maps/sets #4266

adamchalmers opened this issue Nov 9, 2023 · 5 comments · Fixed by #4295

Comments

@adamchalmers
Copy link
Contributor

Hi! The databake crate seems really helpful. One quick question, I notice that Bake is implemented for a lot of stdlib types, mostly strings, numbers, smart pointers and vec/slice/tuple. I notice there's no implementation for sets or maps (either tree- or hash-based).

I imagine there's some reason why Bake isn't implemented for those types. Or maybe it's just that nobody's gotten around to it yet. If there's no reason making this difficult, I think it should be implemented for those types. But I assume there's something going on that I'm not aware of which makes it difficult.

adamchalmers added a commit to KittyCAD/modeling-app that referenced this issue Nov 9, 2023
Databake doesn't have any derive for HashMap, so for NonCodeMeta I decided to skip serializing the non_code_nodes. This should be OK for now.

See <unicode-org/icu4x#4266> for the Databake hashmap issue.
adamchalmers added a commit to KittyCAD/modeling-app that referenced this issue Nov 9, 2023
Databake doesn't have any derive for HashMap, so for NonCodeMeta I decided to skip serializing the non_code_nodes. This should be OK for now.

See <unicode-org/icu4x#4266> for the Databake hashmap issue.
adamchalmers added a commit to KittyCAD/modeling-app that referenced this issue Nov 9, 2023
Databake doesn't have any derive for HashMap, so for NonCodeMeta I decided to skip serializing the non_code_nodes. This should be OK for now.

See <unicode-org/icu4x#4266> for the Databake hashmap issue.
This was referenced Nov 9, 2023
@adamchalmers
Copy link
Contributor Author

Apparently I was overthinking it, and the team just didn't have a need for Bake impls on these collection types. So I'm adding them!

@sffc
Copy link
Member

sffc commented Nov 13, 2023

I think we should add these impls; thanks for the contribution!

However, it should be noted that databaking these more complex collection types will result in some allocations and startup cost (the impls won't be free), at least until the Rust standard library exposes lower-level constructors. We have Bake impls for collections such as LiteMap and ZeroMap which are basically free and supported in const mode. Both of these types support O(log N) lookup, the same asymptotic complexity as BTreeMap.

@adamchalmers
Copy link
Contributor Author

Makes sense. I actually only want HashMap impl for my specific project, but it does seem like we should support more collections if possible.

My usecase actually has nothing to do with performance, I just want an easy way to output my Rust types from a macro. But maybe we could just add docs about the underlying algorithmic complexity.

@robertbastian robertbastian changed the title Impl Databake for maps/sets (or explain why it can't be done) Impl Databake for maps/sets Nov 13, 2023
@robertbastian
Copy link
Member

@adamchalmers we're planning to do a release ~tomorrow, if you want hashmap to be included I suggest you open a PR soon.

@adamchalmers
Copy link
Contributor Author

@adamchalmers we're planning to do a release ~tomorrow, if you want hashmap to be included I suggest you open a PR soon.

Not gonna make it in by tomorrow, don't wait on me! I'm happy to use my fork if I need to :)

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 a pull request may close this issue.

3 participants