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

#![no_std] as an optional feature #59

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

piotrfila
Copy link

This PR makes primal available on no_std targets by adding no-std feature.
I have checked this builds on thumbv7em-none-eabihf.
I ran cargo bench before and after those changes and found no difference in performance (tested on x86_64-linux-gnu).
Without the no-std feature enabled the crate still builds on the MSRV (1.36).

One issue I found is the hamming dependency - no_std support was added there 5 years ago, but the newest version on crates.io is from 8 years ago.

@piotrfila
Copy link
Author

@huonw any chance of merging? I was working on no-std support for another crate and this is a dependency. I would appreciate the changes being published to crates.io so that the downstream crate can also be published.

@cuviper
Copy link
Collaborator

cuviper commented Jan 19, 2024

I can help with changes in this repo, but I don't have access to hamming.

@skewballfox
Copy link

skewballfox commented Jun 9, 2024

@cuviper would removing or replacing hamming work? it seems the only function you are calling from hamming is hamming::weight which is about 35 lines and doesn't seem to have dependencies

@cuviper
Copy link
Collaborator

cuviper commented Jun 12, 2024

Sure -- that also uses its own align_to, but I think we just use the standard slice::align_to now.

primal-bit/src/lib.rs Outdated Show resolved Hide resolved
primal-check/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

I would generally prefer a positive "std" feature, so features remain fully additive, but it would be a breaking change for the current default-features = false case. But we can do it as "no-std" for now, and since there's no lost functionality with this feature it should still be okay.

@skewballfox
Copy link

@piotrfila still interested in working on this? If not, I can try to take over for this PR.

@piotrfila
Copy link
Author

@skewballfox feel free, I've moved on to other things:)

I agree that it's usually better to have a std rather than no-std feature. I chose to do it this way so that libm would only be included for no-std builds (maybe libm is a better name for the feature?).

I think not breaking --no-default-features use cases is not a huge priority, it's normal for crates to move functionality into features and the default feature exists to allow backwards compatibility. If you decide not to use that, it becomes your responsibility to manage features across dep updates.

@cuviper
Copy link
Collaborator

cuviper commented Jun 12, 2024

I agree that it's usually better to have a std rather than no-std feature. I chose to do it this way so that libm would only be included for no-std builds (maybe libm is a better name for the feature?).

The way it's done in num-traits is that with no features, you just don't get parts of the API at all, like Float. Then libm or std features will enable that API, with the latter being preferred in the implementation if both are enabled.

I think not breaking --no-default-features use cases is not a huge priority,

I would rather have a semver bump here to manage any breaking change -- which is fine!

@skewballfox
Copy link

I think I incorporated all the changes discussed. let me know if I missed anything, or need to change anything prior to resolving the mrege conflicts with main

@skewballfox
Copy link

@cuviper sorry to ping you but I can't mark requests for changes as resolved. Can you look over this, see what needs to change prior to resolving the merge conflicts with main?

@skewballfox
Copy link

hey heads up, there's currently a bug with primal check I'm trying to fix. cargo test --no-default-features features libm current fails during compilation despite cargo build --no-default-features features libm working. once it's fixed I'll post information here for testing

@skewballfox
Copy link

skewballfox commented Jun 22, 2024

realized the issue was I accidentally set no_std and didn't feature gate it.

I think this is ready (beside the merge conflicts)

  • cargo test --all passes
  • cargo test --all no-default-features passes
  • cargo test --all no-default-features features libm passes
  • if features libm imports and functions are only used if std is disabled
  • functions which require either are feature gated

If you want I can go ahead and resolve the merge conflicts, or I can wait until you've reviewed the current changes. Same with running cargo fmt --all

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.

3 participants