Skip to content

Commit

Permalink
apply review feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
RalfJung committed Dec 2, 2024
1 parent 41ab0fc commit 8a639d4
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 17 deletions.
8 changes: 5 additions & 3 deletions compiler/rustc_codegen_llvm/src/llvm_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -714,12 +714,14 @@ pub(crate) fn global_llvm_features(
sess.dcx().emit_warn(unknown_feature);
}
Some((_, stability, _)) => {
if let Err(reason) = stability.compute(&sess.target).allow_toggle() {
if let Err(reason) =
stability.compute_toggleability(&sess.target).allow_toggle()
{
sess.dcx().emit_warn(ForbiddenCTargetFeature { feature, reason });
} else if stability.requires_nightly().is_some() {
// An unstable feature. Warn about using it. (It makes little sense
// An unstable feature. Warn about using it. It makes little sense
// to hard-error here since we just warn about fully unknown
// features above).
// features above.
sess.dcx().emit_warn(UnstableCTargetFeature { feature });
}
}
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_codegen_ssa/src/target_features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,14 +153,14 @@ pub(crate) fn provide(providers: &mut Providers) {
// rustdoc needs to be able to document functions that use all the features, so
// whitelist them all
rustc_target::target_features::all_rust_features()
.map(|(a, b)| (a.to_string(), b.compute(target)))
.map(|(a, b)| (a.to_string(), b.compute_toggleability(target)))
.collect()
} else {
tcx.sess
.target
.rust_target_features()
.iter()
.map(|&(a, b, _)| (a.to_string(), b.compute(target)))
.map(|&(a, b, _)| (a.to_string(), b.compute_toggleability(target)))
.collect()
}
},
Expand Down
34 changes: 22 additions & 12 deletions compiler/rustc_target/src/target_features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,17 @@ pub const RUSTC_SPECIFIC_FEATURES: &[&str] = &["crt-static"];
pub const RUSTC_SPECIAL_FEATURES: &[&str] = &["backchain"];

/// Stability information for target features.
/// `AllowToggle` is the type storing whether (un)stable features can be toggled:
/// `Toggleability` is the type storing whether (un)stable features can be toggled:
/// this is initially a function since it can depend on `Target`, but for stable hashing
/// it needs to be something hashable to we have to make the type generic.
#[derive(Debug, Clone, Copy)]
pub enum Stability<AllowToggle> {
pub enum Stability<Toggleability> {
/// This target feature is stable, it can be used in `#[target_feature]` and
/// `#[cfg(target_feature)]`.
Stable {
/// When enabling/dsiabling the feature via `-Ctarget-feature` or `#[target_feature]`,
/// When enabling/disabling the feature via `-Ctarget-feature` or `#[target_feature]`,
/// determine if that is allowed.
allow_toggle: AllowToggle,
allow_toggle: Toggleability,
},
/// This target feature is unstable. It is only present in `#[cfg(target_feature)]` on
/// nightly and using it in `#[target_feature]` requires enabling the given nightly feature.
Expand All @@ -36,7 +36,7 @@ pub enum Stability<AllowToggle> {
/// feature gate!
nightly_feature: Symbol,
/// See `Stable::allow_toggle` comment above.
allow_toggle: AllowToggle,
allow_toggle: Toggleability,
},
/// This feature can not be set via `-Ctarget-feature` or `#[target_feature]`, it can only be
/// set in the basic target definition. It is never set in `cfg(target_feature)`. Used in
Expand All @@ -50,7 +50,7 @@ pub type StabilityUncomputed = Stability<fn(&Target) -> Result<(), &'static str>
/// `Stability` where `allow_toggle` has already been computed.
pub type StabilityComputed = Stability<Result<(), &'static str>>;

impl<CTX, AllowToggle: HashStable<CTX>> HashStable<CTX> for Stability<AllowToggle> {
impl<CTX, Toggleability: HashStable<CTX>> HashStable<CTX> for Stability<Toggleability> {
#[inline]
fn hash_stable(&self, hcx: &mut CTX, hasher: &mut StableHasher) {
std::mem::discriminant(self).hash_stable(hcx, hasher);
Expand All @@ -69,15 +69,22 @@ impl<CTX, AllowToggle: HashStable<CTX>> HashStable<CTX> for Stability<AllowToggl
}
}

impl<AllowToggle> Stability<AllowToggle> {
/// Returns whether the feature can be queried in `cfg` ever.
/// (It might still be nightly-only even if this returns `true`).
impl<Toggleability> Stability<Toggleability> {
/// Returns whether the feature can be used in `cfg(target_feature)` ever.
/// (It might still be nightly-only even if this returns `true`, so make sure to also check
/// `requires_nightly`.)
pub fn in_cfg(self) -> bool {
!matches!(self, Stability::Forbidden { .. })
}

/// Returns the nightly feature that is required to toggle or query this target feature. Ensure
/// to also check `allow_toggle()` before allowing to toggle!
/// Returns the nightly feature that is required to toggle this target feature via
/// `#[target_feature]`/`-Ctarget-feature` or to test it via `cfg(target_feature)`.
/// (For `cfg` we only care whether the feature is nightly or not, we don't require
/// the feature gate to actually be enabled when using a nightly compiler.)
///
/// Before calling this, ensure the feature is even permitted for this use:
/// - for `#[target_feature]`/`-Ctarget-feature`, check `allow_toggle()`
/// - for `cfg(target_feature)`, check `in_cfg`
pub fn requires_nightly(self) -> Option<Symbol> {
match self {
Stability::Unstable { nightly_feature, .. } => Some(nightly_feature),
Expand All @@ -88,7 +95,7 @@ impl<AllowToggle> Stability<AllowToggle> {
}

impl StabilityUncomputed {
pub fn compute(self, target: &Target) -> StabilityComputed {
pub fn compute_toggleability(self, target: &Target) -> StabilityComputed {
use Stability::*;
match self {
Stable { allow_toggle } => Stable { allow_toggle: allow_toggle(target) },
Expand All @@ -101,6 +108,9 @@ impl StabilityUncomputed {
}

impl StabilityComputed {
/// Returns whether the feature may be toggled via `#[target_feature]` or `-Ctarget-feature`.
/// (It might still be nightly-only even if this returns `true`, so make sure to also check
/// `requires_nightly`.)
pub fn allow_toggle(self) -> Result<(), &'static str> {
match self {
Stability::Stable { allow_toggle } => allow_toggle,
Expand Down

0 comments on commit 8a639d4

Please sign in to comment.