Skip to content
This repository was archived by the owner on Nov 30, 2022. It is now read-only.

Conversation

devrandom
Copy link
Contributor

@devrandom devrandom commented May 28, 2021

Direct core2 support.

For context, see discussions in #127

Note that the no-std feature enables core2 and alloc, but you can still compile without either std or no-std, to eliminate the alloc dependency.

@devrandom
Copy link
Contributor Author

Compatible with rust-bitcoin/rust-bitcoin#603

Copy link
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

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

Looks pretty good, much simpler than the other one 😅 One question that came to my mind: do we really need alloc for everything we do with core2 or could that even be a separate feature? At least our hash impls should not allocate imo.

@devrandom
Copy link
Contributor Author

Yes, it's definitely possible to have an alloc feature that is additive to the core2 feature.

@devrandom
Copy link
Contributor Author

I'll followup on the alloc stuff soon

@devrandom
Copy link
Contributor Author

OK, I added the alloc feature

Cargo.toml Outdated
std = []
# The no-std feature enables core2 and the alloc feature adds the alloc crate to that.
# You can still just disable std by disabling default features, without enabling these two.
no-std = ["core2"]
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this as a feature? Users should be able to specify the core2 feature without this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed, although I'm not sure how to best communicate to the developer what are the main features they should be interested in. I expanded the comment.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how to communicate features at all - they aren't really mentioned in anything, and just using Cargo.toml doesn't really solve it. Ideally we'd put it in the top-line lib.rs docs.

Copy link
Member

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

LGTM, aside from dropping the no-std feature.

Copy link
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

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

Looks good. Could use some squashing, at least the little alloc fixup.

@devrandom
Copy link
Contributor Author

ugh, Rust 1.29

@sgeisler
Copy link
Contributor

sgeisler commented Jun 8, 2021

I think there was a trick for that, was it importing std as alloc in the default case?

@devrandom
Copy link
Contributor Author

OK, maybe this will do it

Copy link
Member

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

LGTM. I think maybe we should start documenting all available features in top lib.rs comments, but that isn't specific to this PR, just something I think all our crates should do.

@devrandom
Copy link
Contributor Author

squashed

Copy link
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

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

ACK 5ab0f4b

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ack 5ab0f4b

@TheBlueMatt TheBlueMatt merged commit 5072646 into rust-bitcoin:master Jun 8, 2021
@apoelstra apoelstra mentioned this pull request Jun 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants