From 726819e8eee73ba1ee9ce95606e257c18bf61912 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20H=C3=B8rl=C3=BCck=20Berg?= <36937807+henrikhorluck@users.noreply.github.com> Date: Sun, 9 Jul 2023 22:33:02 +0200 Subject: [PATCH] Clean up feature flags API This also cleans up and removes unnecessary usage of FFI-oriented `feature_metadata_t`, which is only used from Rust code after `builtins/status` was ported. --- fish-rust/src/builtins/status.rs | 16 +-- fish-rust/src/future_feature_flags.rs | 140 ++++++++++---------------- src/fish.cpp | 4 +- src/fish_indent.cpp | 2 +- src/fish_tests.cpp | 19 ++-- src/future_feature_flags.h | 1 - 6 files changed, 73 insertions(+), 109 deletions(-) diff --git a/fish-rust/src/builtins/status.rs b/fish-rust/src/builtins/status.rs index 36db7e896..ed56e7f95 100644 --- a/fish-rust/src/builtins/status.rs +++ b/fish-rust/src/builtins/status.rs @@ -10,7 +10,7 @@ get_job_control_mode, get_login, is_interactive_session, job_control_t, parser_t, set_job_control_mode, Repin, }; -use crate::future_feature_flags::{feature_metadata, feature_test}; +use crate::future_feature_flags::{self as features, feature_test}; use crate::wchar::{wstr, L}; use crate::wchar_ffi::{AsWstr, WCharFromFFI}; use crate::wgetopt::{ @@ -180,10 +180,10 @@ fn default() -> Self { fn print_features(streams: &mut io_streams_t) { // TODO: move this to features.rs let mut max_len = i32::MIN; - for md in feature_metadata() { + for md in &features::METADATA { max_len = max_len.max(md.name.len() as i32); } - for md in feature_metadata() { + for md in &features::METADATA { let set = if feature_test(md.flag) { L!("on") } else { @@ -192,10 +192,10 @@ fn print_features(streams: &mut io_streams_t) { streams.out.append(sprintf!( "%-*ls%-3s %ls %ls\n", max_len + 1, - md.name.as_wstr(), + md.name, set, - md.groups.as_wstr(), - md.description.as_wstr(), + md.groups, + md.description, )); } } @@ -433,8 +433,8 @@ pub fn status( } use TestFeatureRetVal::*; let mut retval = Some(TEST_FEATURE_NOT_RECOGNIZED as c_int); - for md in &feature_metadata() { - if md.name.as_wstr() == args[0] { + for md in &features::METADATA { + if md.name == args[0] { retval = match feature_test(md.flag) { true => Some(TEST_FEATURE_ON as c_int), false => Some(TEST_FEATURE_OFF as c_int), diff --git a/fish-rust/src/future_feature_flags.rs b/fish-rust/src/future_feature_flags.rs index 4ff90c11c..f2e4cdf5f 100644 --- a/fish-rust/src/future_feature_flags.rs +++ b/fish-rust/src/future_feature_flags.rs @@ -2,8 +2,6 @@ use crate::ffi::wcharz_t; use crate::wchar::wstr; -use crate::wchar_ffi::WCharToFFI; -use std::array; use std::sync::atomic::AtomicBool; use std::sync::atomic::Ordering; use widestring_suffix::widestrs; @@ -31,74 +29,49 @@ enum FeatureFlag { ampersand_nobg_in_token, } - /// Metadata about feature flags. - struct feature_metadata_t { - flag: FeatureFlag, - name: UniquePtr, - groups: UniquePtr, - description: UniquePtr, - default_value: bool, - read_only: bool, - } - extern "Rust" { - type Features; - fn test(self: &Features, flag: FeatureFlag) -> bool; - fn set(self: &mut Features, flag: FeatureFlag, value: bool); - fn set_from_string(self: &mut Features, str: wcharz_t); - fn fish_features() -> *const Features; - fn feature_test(flag: FeatureFlag) -> bool; - fn mutable_fish_features() -> *mut Features; - fn feature_metadata() -> [feature_metadata_t; 4]; + #[cxx_name = "feature_test"] + fn test(flag: FeatureFlag) -> bool; + #[cxx_name = "feature_set"] + fn set(flag: FeatureFlag, value: bool); + #[cxx_name = "feature_set_from_string"] + fn set_from_string(str: wcharz_t); } } -pub use future_feature_flags_ffi::{feature_metadata_t, FeatureFlag}; +pub use future_feature_flags_ffi::FeatureFlag; -pub struct Features { +struct Features { // Values for the flags. // These are atomic to "fix" a race reported by tsan where tests of feature flags and other // tests which use them conceptually race. - values: [AtomicBool; metadata.len()], + values: [AtomicBool; METADATA.len()], } /// Metadata about feature flags. -struct FeatureMetadata { +pub struct FeatureMetadata { /// The flag itself. - flag: FeatureFlag, + pub flag: FeatureFlag, /// User-presentable short name of the feature flag. - name: &'static wstr, + pub name: &'static wstr, /// Comma-separated list of feature groups. - groups: &'static wstr, + pub groups: &'static wstr, /// User-presentable description of the feature flag. - description: &'static wstr, + pub description: &'static wstr, /// Default flag value. - default_value: bool, + pub default_value: bool, /// Whether the value can still be changed or not. - read_only: bool, -} - -impl From<&FeatureMetadata> for feature_metadata_t { - fn from(md: &FeatureMetadata) -> feature_metadata_t { - feature_metadata_t { - flag: md.flag, - name: md.name.to_ffi(), - groups: md.groups.to_ffi(), - description: md.description.to_ffi(), - default_value: md.default_value, - read_only: md.read_only, - } - } + pub read_only: bool, } /// The metadata, indexed by flag. #[widestrs] -const metadata: [FeatureMetadata; 4] = [ +pub const METADATA: [FeatureMetadata; 4] = [ FeatureMetadata { flag: FeatureFlag::stderr_nocaret, name: "stderr-nocaret"L, @@ -134,36 +107,52 @@ fn from(md: &FeatureMetadata) -> feature_metadata_t { ]; /// The singleton shared feature set. -static mut global_features: Features = Features::new(); +static FEATURES: Features = Features::new(); + +/// Perform a feature test on the global set of features. +pub fn test(flag: FeatureFlag) -> bool { + FEATURES.test(flag) +} + +pub fn feature_test(flag: FeatureFlag) -> bool { + test(flag) +} + +/// Set a flag. +pub fn set(flag: FeatureFlag, value: bool) { + FEATURES.set(flag, value); +} + +/// Parses a comma-separated feature-flag string, updating ourselves with the values. +/// Feature names or group names may be prefixed with "no-" to disable them. +/// The special group name "all" may be used for those who like to live on the edge. +/// Unknown features are silently ignored. +pub fn set_from_string<'a>(str: impl Into<&'a wstr>) { + FEATURES.set_from_string(str); +} impl Features { const fn new() -> Self { Features { values: [ - AtomicBool::new(metadata[0].default_value), - AtomicBool::new(metadata[1].default_value), - AtomicBool::new(metadata[2].default_value), - AtomicBool::new(metadata[3].default_value), + AtomicBool::new(METADATA[0].default_value), + AtomicBool::new(METADATA[1].default_value), + AtomicBool::new(METADATA[2].default_value), + AtomicBool::new(METADATA[3].default_value), ], } } - /// Return whether a flag is set. - pub fn test(&self, flag: FeatureFlag) -> bool { + fn test(&self, flag: FeatureFlag) -> bool { self.values[flag.repr as usize].load(Ordering::SeqCst) } - /// Set a flag. - pub fn set(&mut self, flag: FeatureFlag, value: bool) { + fn set(&self, flag: FeatureFlag, value: bool) { self.values[flag.repr as usize].store(value, Ordering::SeqCst) } - /// Parses a comma-separated feature-flag string, updating ourselves with the values. - /// Feature names or group names may be prefixed with "no-" to disable them. - /// The special group name "all" may be used for those who like to live on the edge. - /// Unknown features are silently ignored. #[widestrs] - pub fn set_from_string<'a>(&mut self, str: impl Into<&'a wstr>) { + fn set_from_string<'a>(&self, str: impl Into<&'a wstr>) { let str: &wstr = str.into(); let whitespace = "\t\n\0x0B\0x0C\r "L.as_char_slice(); for entry in str.as_char_slice().split(|c| *c == ',') { @@ -186,14 +175,14 @@ pub fn set_from_string<'a>(&mut self, str: impl Into<&'a wstr>) { // is to allow uniform invocations of fish (e.g. disable a feature that is only present in // future versions). // The special name 'all' may be used for those who like to live on the edge. - if let Some(md) = metadata.iter().find(|md| md.name == name) { + if let Some(md) = METADATA.iter().find(|md| md.name == name) { // Only change it if it's not read-only. // Don't complain if it is, this is typically set from a variable. if !md.read_only { self.set(md.flag, value); } } else { - for md in &metadata { + for md in &METADATA { if md.groups == name || name == "all"L { if !md.read_only { self.set(md.flag, value); @@ -205,41 +194,18 @@ pub fn set_from_string<'a>(&mut self, str: impl Into<&'a wstr>) { } } -/// Return the global set of features for fish. -pub fn fish_features() -> &'static Features { - // Safety: this will become const with atomics after Rust conversion. - unsafe { &global_features } -} - -/// Perform a feature test on the global set of features. -pub fn feature_test(flag: FeatureFlag) -> bool { - fish_features().test(flag) -} - -/// Return the global set of features for fish, but mutable. In general fish features should be set -/// at startup only. -pub fn mutable_fish_features() -> *mut Features { - // Safety: this will be ported to use atomics after Rust conversion. - unsafe { &mut global_features as *mut Features } -} - -// The metadata, indexed by flag. -pub fn feature_metadata() -> [feature_metadata_t; metadata.len()] { - array::from_fn(|i| (&metadata[i]).into()) -} - #[test] #[widestrs] fn test_feature_flags() { - let mut f = Features::new(); + let f = Features::new(); f.set_from_string("stderr-nocaret,nonsense"L); assert!(f.test(FeatureFlag::stderr_nocaret)); f.set_from_string("stderr-nocaret,no-stderr-nocaret,nonsense"L); assert!(f.test(FeatureFlag::stderr_nocaret)); // Ensure every metadata is represented once. - let mut counts: [usize; metadata.len()] = [0; metadata.len()]; - for md in &metadata { + let mut counts: [usize; METADATA.len()] = [0; METADATA.len()]; + for md in &METADATA { counts[md.flag.repr as usize] += 1; } for count in counts { @@ -247,7 +213,7 @@ fn test_feature_flags() { } assert_eq!( - metadata[FeatureFlag::stderr_nocaret.repr as usize].name, + METADATA[FeatureFlag::stderr_nocaret.repr as usize].name, "stderr-nocaret"L ); } diff --git a/src/fish.cpp b/src/fish.cpp index fb7ccb83e..4e172d6ca 100644 --- a/src/fish.cpp +++ b/src/fish.cpp @@ -500,10 +500,10 @@ int main(int argc, char **argv) { // command line takes precedence). if (auto features_var = env_stack_t::globals().get(L"fish_features")) { for (const wcstring &s : features_var->as_list()) { - mutable_fish_features()->set_from_string(s.c_str()); + feature_set_from_string(s.c_str()); } } - mutable_fish_features()->set_from_string(opts.features.c_str()); + feature_set_from_string(opts.features.c_str()); proc_init(); misc_init(); reader_init(); diff --git a/src/fish_indent.cpp b/src/fish_indent.cpp index 972a25127..37af1ab74 100644 --- a/src/fish_indent.cpp +++ b/src/fish_indent.cpp @@ -300,7 +300,7 @@ int main(int argc, char *argv[]) { if (auto features_var = env_stack_t::globals().get(L"fish_features")) { for (const wcstring &s : features_var->as_list()) { - mutable_fish_features()->set_from_string(s.c_str()); + feature_set_from_string(s.c_str()); } } diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 571720a14..e96e99edb 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -2473,15 +2473,14 @@ static void test_wildcards() { unescape_string_in_place(&wc, UNESCAPE_SPECIAL); do_test(!wildcard_has(wc) && wildcard_has_internal(wc)); - auto feat = mutable_fish_features(); - auto saved = feat->test(feature_flag_t::qmark_noglob); - feat->set(feature_flag_t::qmark_noglob, false); + auto saved = feature_test(feature_flag_t::qmark_noglob); + feature_set(feature_flag_t::qmark_noglob, false); do_test(wildcard_has(L"?")); do_test(!wildcard_has(L"\\?")); - feat->set(feature_flag_t::qmark_noglob, true); + feature_set(feature_flag_t::qmark_noglob, true); do_test(!wildcard_has(L"?")); do_test(!wildcard_has(L"\\?")); - feat->set(feature_flag_t::qmark_noglob, saved); + feature_set(feature_flag_t::qmark_noglob, saved); } static void test_complete() { @@ -4904,7 +4903,7 @@ static void test_highlighting() { #endif bool saved_flag = feature_test(feature_flag_t::ampersand_nobg_in_token); - mutable_fish_features()->set(feature_flag_t::ampersand_nobg_in_token, true); + feature_set(feature_flag_t::ampersand_nobg_in_token, true); for (const highlight_component_list_t &components : highlight_tests) { // Generate the text. wcstring text; @@ -4949,7 +4948,7 @@ static void test_highlighting() { } } } - mutable_fish_features()->set(feature_flag_t::ampersand_nobg_in_token, saved_flag); + feature_set(feature_flag_t::ampersand_nobg_in_token, saved_flag); vars.remove(L"VARIABLE_IN_COMMAND", ENV_DEFAULT); vars.remove(L"VARIABLE_IN_COMMAND2", ENV_DEFAULT); } @@ -5359,7 +5358,7 @@ static void test_string() { {{L"string", L"match", L"?*", L"a", nullptr}, STATUS_CMD_ERROR, L""}, {{L"string", L"match", L"?*", L"ab", nullptr}, STATUS_CMD_ERROR, L""}, {{L"string", L"match", L"a*\\?", L"abc?", nullptr}, STATUS_CMD_ERROR, L""}}; - mutable_fish_features()->set(feature_flag_t::qmark_noglob, true); + feature_set(feature_flag_t::qmark_noglob, true); for (const auto &t : qmark_noglob_tests) { run_one_string_test(t.argv, t.expected_rc, t.expected_out); } @@ -5371,11 +5370,11 @@ static void test_string() { {{L"string", L"match", L"?*", L"a", nullptr}, STATUS_CMD_OK, L"a\n"}, {{L"string", L"match", L"?*", L"ab", nullptr}, STATUS_CMD_OK, L"ab\n"}, {{L"string", L"match", L"a*\\?", L"abc?", nullptr}, STATUS_CMD_OK, L"abc?\n"}}; - mutable_fish_features()->set(feature_flag_t::qmark_noglob, false); + feature_set(feature_flag_t::qmark_noglob, false); for (const auto &t : qmark_glob_tests) { run_one_string_test(t.argv, t.expected_rc, t.expected_out); } - mutable_fish_features()->set(feature_flag_t::qmark_noglob, saved_flag); + feature_set(feature_flag_t::qmark_noglob, saved_flag); } /// Helper for test_timezone_env_vars(). diff --git a/src/future_feature_flags.h b/src/future_feature_flags.h index 29f91f0c2..a748a6324 100644 --- a/src/future_feature_flags.h +++ b/src/future_feature_flags.h @@ -3,6 +3,5 @@ #include "future_feature_flags.rs.h" using feature_flag_t = FeatureFlag; -using features_t = Features; #endif