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

Provide Amount & co in no-alloc #2408

Merged
merged 2 commits into from
Jan 28, 2024
Merged

Conversation

Kixunil
Copy link
Collaborator

@Kixunil Kixunil commented Jan 27, 2024

Using the crate without allocation was previously disabled making the
crate empty without the feature. This chage makes it more fine-grained:
it only disables string and float conversions which use allocator. We
could later provide float conversions by using a sufficiently-long
ArrayString.

Note that this is API-breaking because we disallow calling the methods of the sealed SerdeAmount trait. However I think it should've been obvious that the thing is internal and calling them is not a great idea. (BTW I only learned this trick recently).

Closes #2389

Using the crate without allocation was previously disabled making the
crate empty without the feature. This chage makes it more fine-grained:
it only disables string and float conversions which use allocator. We
could later provide float conversions by using a sufficiently-long
`ArrayString`.
@Kixunil Kixunil added API break This PR requires a version bump for the next release 1.0 Issues and PRs required or helping to stabilize the API labels Jan 27, 2024
@github-actions github-actions bot added the C-units PRs modifying the units crate label Jan 27, 2024
@coveralls
Copy link

coveralls commented Jan 27, 2024

Pull Request Test Coverage Report for Build 7678592996

  • -8 of 45 (82.22%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 84.308%

Changes Missing Coverage Covered Lines Changed/Added Lines %
units/src/amount.rs 37 45 82.22%
Totals Coverage Status
Change from base Build 7668596887: 0.001%
Covered Lines: 19315
Relevant Lines: 22910

💛 - Coveralls

Previously the crate used negative reasoning to enable `std` which was
hard to understand, required the `prelude` module and wasn't really
needed because it's only needed when a crate wants to add `alloc`
feature-backwards compatibly and this crate always had the feature.

This cleans up usage to unconditionally use `#[no_std]` and then just
add `extern crate` on top as needed by activated features.
@apoelstra
Copy link
Member

Fun! I also learned this trick recently (maybe from a cargo-semver-checks blog post).

The use of #[no_std] is new to me ... a long time ago I knew about this attribute but I don't know what it does.

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 ac26171 maaybe private should be renamed to internal_api or something

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jan 27, 2024

maybe from a cargo-semver-checks blog post

That's definitely where I found it.

The use of #[no_std] is new to me ... a long time ago I knew about this attribute but I don't know what it does.

Without it each crate has an implicit extern crate std injected with the std prelude etc. The attribute turns it off, so then you need to manually import extern crate std if you want and import prelude items. One approach is to do exactly that in no_std libraries with std enabled, the other is to use not(std) on the attribute and a bunch of other places.

Personally I find it easier to reason about additive features. When something requires feature x just slap feature = "x" there and it's done. Also seeing #[no_std] immediately, not somewhere between other attributes is nice for discoverability.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jan 27, 2024

maaybe private should be renamed to internal_api or something

Usually it's called sealed but I didn't think it's important to change the name.

@apoelstra
Copy link
Member

Usually it's called sealed but I didn't think it's important to change the name.

Well, by definition we can rename it without breaking the API so I'm not gonna hold up this PR over it :). But if in the future somebody suggests a clearly-better or well-established name we should move to it.

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK ac26171

@apoelstra apoelstra merged commit d883c3d into rust-bitcoin:master Jan 28, 2024
31 checks passed
@Kixunil Kixunil deleted the amount-no-alloc branch January 29, 2024 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0 Issues and PRs required or helping to stabilize the API API break This PR requires a version bump for the next release C-units PRs modifying the units crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

units: Make amount module not require "alloc" feature
4 participants