search

rust-embedded-community / usb-device

Experimental device-side USB framework for microcontrollers in Rust.
MIT License
451 stars 76 forks source link

Feature gate portable atomics #148

Open MarcusGrass opened 7 months ago

MarcusGrass commented 7 months ago

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

mvirkkunen commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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.