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

feat: add alloc feature #56

Closed
wants to merge 2 commits into from
Closed

feat: add alloc feature #56

wants to merge 2 commits into from

Conversation

pmnoxx
Copy link
Contributor

@pmnoxx pmnoxx commented Dec 17, 2021

Rust is built around the concept of “zero cost abstraction”.

It's best not to hide extra allocation costs. I see, that by default there is an implementation available for HashSet and HashMap. It's a good idea to put it behind use_alloc feature.

In order not to break backwards compatibility, I added it to default_features.

Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

borsh already supports no_std, and this feature works correctly. On no-std, HasMap and HashSet are hashbrown variations. This itself is somewhat questionable design descisions though.

Note that HashMap isn't actually a part of alloc crate, as it uses access to randomness, which is not a part of alloc.

@@ -63,7 +61,7 @@ pub struct BorshSchemaContainer {
/// Declaration of the type.
pub declaration: Declaration,
/// All definitions needed to deserialize the given type.
pub definitions: HashMap<Declaration, Definition>,
pub definitions: Vec<(Declaration, Definition)>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes a public API, this shouldn't be done lightly. See #51 for the context on things which could be better, but which we can't just do, as that'll break the world.

@pmnoxx pmnoxx closed this Dec 17, 2021
@pmnoxx pmnoxx changed the title feat: add alloc feature feat: add use_alloc feature Dec 17, 2021
@pmnoxx pmnoxx changed the title feat: add use_alloc feature feat: add alloc feature Dec 20, 2021
@pmnoxx
Copy link
Contributor Author

pmnoxx commented Dec 20, 2021

borsh already supports no_std, and this feature works correctly. On no-std, HasMap and HashSet are hashbrown variations. This itself is somewhat questionable design descisions though.

Note that HashMap isn't actually a part of alloc crate, as it uses access to randomness, which is not a part of alloc.

FYI, borsh can't be compiled without std. @matklad

> cargo build --no-default-features

error: cannot find macro `vec` in this scope
   --> borsh/src/de/mod.rs:283:30
    |
283 |             let mut result = vec![T::deserialize(buf)?];
    |                              ^^^
    |
    = note: consider importing one of these items:
            alloc::vec
            crate::maybestd::vec

error: could not compile `borsh` due to previous error

@pmnoxx pmnoxx deleted the piotr-add-alloc-feature branch December 23, 2021 00:03
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