-
Notifications
You must be signed in to change notification settings - Fork 635
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
Use autocfg instead of unstable feature(cfg_target_has_atomic) #2294
Conversation
0a07109
to
627380a
Compare
LGTM
The user doesn't even need to enable this, for anyone not supporting targets like |
c0e926c
to
621ca85
Compare
macro_rules! cfg_target_has_atomic { | ||
($($item:item)*) => {$( | ||
#[cfg_attr(feature = "cfg-target-has-atomic", cfg(target_has_atomic = "ptr"))] | ||
#[cfg(not(no_atomic_cas))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unfortunate that we need to use double negation, but AFAIK, this is the easiest way to implement a workaround for the first problem.
621ca85
to
32a22ef
Compare
Another workaround for this is to pass a no-op expression (empty block or unit) to |
32a22ef
to
2c2dde2
Compare
After rust-lang#2299, this is no longer needed.
Just seeing this now. I'm very familiar with the hassle that custom It's easy for custom build systems to enable a feature flag, but running arbitrary other rust code (
I'm not totally sure I understand the benefits of this change. These unstable feature usages are already gated on an |
Do you mean adding something like indexmap-rs/indexmap#145 and rust-num/num-traits#185? If so, it's definitely possible.
I think it's nice to remove dependencies on unstable features, whether explicitly gated or not.
|
Detect platforms that do not support AtomicU64 by using the same way. AFAIK, this is more robust than the current way that uses autocfg. See also rust-lang/futures-rs#2294.
Detect platforms that do not support AtomicU64 by using the same way. AFAIK, this is more robust than the current way that uses autocfg. See also rust-lang/futures-rs#2294.
Detect platforms that do not support AtomicU64 by using the same way. AFAIK, this is more robust than the current way that uses autocfg. See also rust-lang/futures-rs#2294.
Detect platforms that do not support AtomicU64 by using the same way. AFAIK, this is more robust than the current way that uses autocfg. See also rust-lang/futures-rs#2294.
Detect platforms that do not support AtomicU64 by using the same way. AFAIK, this is more robust than the current way that uses autocfg. See also rust-lang/futures-rs#2294.
Detect platforms that do not support AtomicU64 by using the same way. AFAIK, this is more robust than the current way that uses autocfg. See also rust-lang/futures-rs#2294.
Instead of depending on unstable features of the compiler, use
autocfg
crate to detect if atomic CAS is available.This completely removes the dependency on unstable features from crates other than futures and futures-util.
I'm planning to remove the dependency on unstable features from all crates, which is the first step in that plan.
EDIT: See #2294 (comment)feature(cfg_target_has_atomic)
is propagated to thecore
/alloc
imported in all dependencies if any of the dependencies graph crates are usesfeature(cfg_target_has_atomic)
, so it works if the user just enables thefeature(cfg_target_has_atomic)
.(Previously you had to enable both
feature(cfg_target_has_atomic)
andfutures
crate'scfg-target-has-atomic
feature.)Unfortunately, this has some (potential) concerns about breakage:
If the build script is not called on these build systems, modules/types that require
cfg(target_has_atomic = "ptr")
, such as channels, will not compile. However, I think this can workaround by always treatingcfg(target_has_atomic = "ptr")
as true when the build script doesn't run. (This should behave exactly the same as currentfutures
withoutcfg-target-has-atomic
feature.)Also, I haven't actually tested it, but Bazel (cargo-raze) and Buck(reindeer) seem to support build scripts, so I'm not too worried about this.
autocfg
is lightweight and widely used, and this is a much smaller increase compared to time reduced by futures-util: Migrate from pin-project to pin-project-lite #2273, so I don't think this is really a problem.cfg(target_has_atomic = "ptr")
will not compile ifautocfg
does not work as expected. See 1.48 beta broken on mips due to copy_file_range EOVERFLOW rust#78979, IndexMap compilation fails "expected 3 type arguments" indexmap-rs/indexmap#151, 1.5.0 breaks h2 build indexmap-rs/indexmap#144, 1.29 fails to build on a Windows networked drive rust#54216, Can no longer build on custom target rust-num/num-traits#177, i128 no_std rust-num/num-complex#83 for the actual case where similar issue occurred.A known workaround for this seems to be to add a cargo feature that always treats
cfg(target_has_atomic = "ptr")
as true, like Add a feature to explicitly enable "std" indexmap-rs/indexmap#145, Trust the "i128" feature rust-num/num-traits#185.I haven't implemented workarounds yet, but I think there are workarounds for possible issues (basically).
Also, the approach I adopt in this PR is basically the same as the one I'm planning to use in the changes proposed in #2207.