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

Feature gate portable atomics #148

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MarcusGrass
Copy link

Allow an opt-out for portable atomics, if the platform has atomic load/store then they aren't necessary

@mvirkkunen
Copy link
Collaborator

mvirkkunen commented Apr 13, 2024

Doesn't portable-atomic already do feature detection and do essentially the same thing automatically?

Edit: Okay, I guess it can be a bit overzealous sometimes if the target doesn't support all atomic operations, but it has its own feature flags to disable that it seems. Could that be used instead?

@MarcusGrass
Copy link
Author

MarcusGrass commented Apr 13, 2024

Doesn't portable-atomic already do feature detection and do essentially the same thing automatically?

Edit: Okay, I guess it can be a bit overzealous sometimes if the target doesn't support all atomic operations, but it has its own feature flags to disable that it seems. Could that be used instead?

I think that from a codegen standpoint it'll just be the same in release, and the unused stuff should all be eliminated, but from a hygiene perspective fewer dependencies would be nicer because of the usual reasons, not really that important though.

I fully wanted this out of my dependency graph to make sure that there weren't any unintended side-effects like intrinsics creation that I relied on implicitly and decided might as well post a patch.
Although, since i noticed a major is coming up, making it opt-in would be even better (from my perspective). But it's just a matter of taste.

Edit:
Just to be really clear, I think it would be completely understandable if you reject this if you feel differently about default deps, I can easily maintain a fork, and if you start using more portable-atomics features in the future and that fork becomes difficult to maintain I'll just switch back.

@ryan-summers
Copy link
Member

ryan-summers commented Apr 15, 2024

While I understand the desire to have portable-atomics out of your individual dependency graph, I'm not sure this is the best approach for the broader audience. The whole point of the portable-atomics dependency is to make atomics work for the user regardless of platform and to enable this crate for all platforms as painlessly as possible.

Given that the end result from a codegen perspective should be the same for platforms where atomics are supported, I don't see a benefit in forcing users to figure out whether or not their platform has atomics and selecting the appropriate feature flags.

I fully understand that wanting to ensure emulated atomics aren't present is a very real design task for many projects, but I don't think that this is the best approach for the wide-ranging audience that this crate supports.

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.

None yet

3 participants