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

Conditionally compile atomic implementations #17

Closed
wants to merge 2 commits into from
Closed

Conditionally compile atomic implementations #17

wants to merge 2 commits into from

Conversation

fwcd
Copy link

@fwcd fwcd commented May 19, 2023

This enables compilation e.g. on 32-bit platforms where 64-bit atomics may be unavailable.

@udoprog
Copy link
Owner

udoprog commented May 19, 2023

Hi!

Since it's using cfg_target_has_atomic, it makes musli nightly only. I think the only option is to put it behind a feature flag the user is responsible for turning off. Unless there's some other way to make it work on stable.

@udoprog udoprog added the enhancement New feature or request label May 19, 2023
@fwcd
Copy link
Author

fwcd commented May 22, 2023

I've updated it to use feature flags. Once rust-lang/rust#94039 stabilizes, we could deprecate the features and revert to the original implementation (a96056c).

@fwcd
Copy link
Author

fwcd commented Jun 26, 2023

Any update on this?

@udoprog
Copy link
Owner

udoprog commented Jun 26, 2023

Looks Ok, I've been working on some other things and haven't come back to this project yet. But I'll rebase this for now.

I might tweak the features before release, but I haven't decided how yet.

@udoprog
Copy link
Owner

udoprog commented Jun 29, 2023

@fwcd So I was looking around how other projects was approaching this, and target_has_atomic is stable since 1.60. So what I did in the last PR was to try and lean on that instead. While the visibility of Atomic* depends on target_has_atomic_load_store (which is nightly only) it seems like there's no reasonable case where the two would diverge.

Could you take the commit I pushed for a spin in your project and see if it works as expected?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants