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

Allow usage on no_std targets and targets using portable-atomic #7

Merged
merged 5 commits into from
Sep 1, 2022

Conversation

notgull
Copy link
Member

@notgull notgull commented Aug 28, 2022

This pull requests replaces this crate's dependency on libstd with libcore, allowing it to be used on no_std targets. In addition, it adds a feature called atomic-polyfill, which uses the crate of the same name to provide AtomicUsize on no_std targets.

@taiki-e
Copy link
Collaborator

taiki-e commented Aug 29, 2022

The atomic-polyfill and the feature-based approach it recommends can very easily introduce unsoundness when the dependency enables that feature. (Even if they no longer provide a default implementation, even if they warn about it in their documentation. -- I have seen many libraries in the async ecosystem where the similar feature to select TLS or runtime is enabled by default or always enabled. So, I believe that such an approach is fundamentally fragile.)

See also mvdnes/spin-rs#114 and tokio-rs/bytes#461 (comment).

@notgull
Copy link
Member Author

notgull commented Aug 29, 2022

I see that in the issues you linked portable-atomic as a potential alternative. Would you be fine with that instead?

@taiki-e
Copy link
Collaborator

taiki-e commented Aug 29, 2022

Yeah, I'm fine with that.

@notgull notgull changed the title Allow usage on no_std targets and targets using atomic-polyfill Allow usage on no_std targets and targets using portable-atomic Aug 29, 2022
@notgull
Copy link
Member Author

notgull commented Aug 29, 2022

I've ported this code to use portable-atomic instead.

src/lib.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@notgull
Copy link
Member Author

notgull commented Aug 31, 2022

I just added a check to the CI that builds on a no_std target.

Copy link
Collaborator

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants