From 3a3474371b6b87e630e7aa217e7860e9154cd563 Mon Sep 17 00:00:00 2001 From: Famiu Haque Date: Tue, 28 Nov 2023 06:15:26 +0600 Subject: [PATCH] refactor(options): replace `p_force_(on|off)` with `immutable` (#26209) Problem: We use the `p_force_on` and `p_force_off` variables to check if a variable is immutable and what its default value is. This is not only hacky and unintuitive, but also is limited to only boolean options. Solution: Replace `p_force_on` and `p_force_off` with an `immutable` property for options, which indicates if an option is immutable. Immutable options cannot be changed from their default value. Ref: #25672. --- src/nvim/generators/gen_options.lua | 4 ++ src/nvim/option.c | 75 +++++++++++++++-------------- src/nvim/option.h | 1 + src/nvim/option_vars.h | 4 -- src/nvim/options.lua | 19 ++++---- 5 files changed, 53 insertions(+), 50 deletions(-) diff --git a/src/nvim/generators/gen_options.lua b/src/nvim/generators/gen_options.lua index c9878bf3b0..26ade2745d 100644 --- a/src/nvim/generators/gen_options.lua +++ b/src/nvim/generators/gen_options.lua @@ -143,9 +143,13 @@ local function dump_option(i, o) end if o.varname then w(' .var=&' .. o.varname) + -- Immutable options should directly point to the default value + elseif o.immutable then + w((' .var=&options[%u].def_val'):format(i - 1)) elseif #o.scope == 1 and o.scope[1] == 'window' then w(' .var=VAR_WIN') end + w(' .immutable=' .. (o.immutable and 'true' or 'false')) if #o.scope == 1 and o.scope[1] == 'global' then w(' .indir=PV_NONE') else diff --git a/src/nvim/option.c b/src/nvim/option.c index 972002585e..eda8ccbfb7 100644 --- a/src/nvim/option.c +++ b/src/nvim/option.c @@ -1913,26 +1913,6 @@ static void apply_optionset_autocmd(int opt_idx, int opt_flags, OptVal oldval, O reset_v_option_vars(); } -/// Ensure that options set to p_force_on cannot be disabled. -static const char *did_set_force_on(optset_T *args FUNC_ATTR_UNUSED) -{ - if (p_force_on == false) { - p_force_on = true; - return e_unsupportedoption; - } - return NULL; -} - -/// Ensure that options set to p_force_off cannot be enabled. -static const char *did_set_force_off(optset_T *args FUNC_ATTR_UNUSED) -{ - if (p_force_off == true) { - p_force_off = false; - return e_unsupportedoption; - } - return NULL; -} - /// Process the updated 'arabic' option value. static const char *did_set_arabic(optset_T *args) { @@ -3195,6 +3175,7 @@ int findoption(const char *const arg) return findoption_len(arg, strlen(arg)); } +/// Free an allocated OptVal. void optval_free(OptVal o) { switch (o.type) { @@ -3211,6 +3192,7 @@ void optval_free(OptVal o) } } +/// Copy an OptVal. OptVal optval_copy(OptVal o) { switch (o.type) { @@ -3224,6 +3206,27 @@ OptVal optval_copy(OptVal o) UNREACHABLE; } +/// Check if two option values are equal. +bool optval_equal(OptVal o1, OptVal o2) +{ + if (o1.type != o2.type) { + return false; + } + + switch (o1.type) { + case kOptValTypeNil: + return true; + case kOptValTypeBoolean: + return o1.data.boolean == o2.data.boolean; + case kOptValTypeNumber: + return o1.data.number == o2.data.number; + case kOptValTypeString: + return o1.data.string.size == o2.data.string.size + && strequal(o1.data.string.data, o2.data.string.data); + } + UNREACHABLE; +} + /// Match type of OptVal with the type of the target option. Returns true if the types match and /// false otherwise. static bool optval_match_type(OptVal o, int opt_idx) @@ -3480,14 +3483,16 @@ OptVal get_option_value(const char *name, uint32_t *flagsp, int scope, bool *hid return NIL_OPTVAL; } - void *varp = get_varp_scope(&(options[opt_idx]), scope); + vimoption_T *opt = &options[opt_idx]; + void *varp = get_varp_scope(opt, scope); + if (hidden != NULL) { *hidden = varp == NULL; } if (flagsp != NULL) { // Return the P_xxxx option flags. - *flagsp = options[opt_idx].flags; + *flagsp = opt->flags; } return optval_copy(optval_from_varp(opt_idx, varp)); @@ -3542,7 +3547,6 @@ static const char *did_set_option(int opt_idx, void *varp, OptVal old_value, Opt bool free_oldval = (opt->flags & P_ALLOCED); bool value_changed = false; - opt_did_set_cb_T did_set_cb; optset_T did_set_cb_args = { .os_varp = varp, .os_idx = opt_idx, @@ -3558,24 +3562,21 @@ static const char *did_set_option(int opt_idx, void *varp, OptVal old_value, Opt .os_win = curwin }; - if ((int *)varp == &p_force_on) { - did_set_cb = did_set_force_on; - } else if ((int *)varp == &p_force_off) { - did_set_cb = did_set_force_off; - } else { - did_set_cb = opt->opt_did_set_cb; + // Disallow changing immutable options. + if (opt->immutable && !optval_equal(old_value, new_value)) { + errmsg = e_unsupportedoption; } - - // Disallow changing some options from secure mode - if ((secure || sandbox != 0) && (opt->flags & P_SECURE)) { + // Disallow changing some options from secure mode. + else if ((secure || sandbox != 0) && (opt->flags & P_SECURE)) { errmsg = e_secure; - // Check for a "normal" directory or file name in some string options. - } else if (new_value.type == kOptValTypeString - && check_illegal_path_names(*(char **)varp, opt->flags)) { + } + // Check for a "normal" directory or file name in some string options. + else if (new_value.type == kOptValTypeString + && check_illegal_path_names(*(char **)varp, opt->flags)) { errmsg = e_invarg; - } else if (did_set_cb != NULL) { + } else if (opt->opt_did_set_cb != NULL) { // Invoke the option specific callback function to validate and apply the new value. - errmsg = did_set_cb(&did_set_cb_args); + errmsg = opt->opt_did_set_cb(&did_set_cb_args); // The 'filetype' and 'syntax' option callback functions may change the os_value_changed field. value_changed = did_set_cb_args.os_value_changed; // The 'keymap', 'filetype' and 'syntax' option callback functions may change the diff --git a/src/nvim/option.h b/src/nvim/option.h index f5cd484fd6..4d89d354f9 100644 --- a/src/nvim/option.h +++ b/src/nvim/option.h @@ -48,6 +48,7 @@ typedef struct vimoption { ///< local option: indirect option index ///< callback function to invoke after an option is modified to validate and ///< apply the new value. + bool immutable; ///< option value cannot be changed from the default value. /// callback function to invoke after an option is modified to validate and /// apply the new value. diff --git a/src/nvim/option_vars.h b/src/nvim/option_vars.h index 45d9e0c31b..bffdfb25ef 100644 --- a/src/nvim/option_vars.h +++ b/src/nvim/option_vars.h @@ -456,7 +456,6 @@ EXTERN unsigned dy_flags; #define DY_UHEX 0x004 // legacy flag, not used #define DY_MSGSEP 0x008 -EXTERN int p_ed; ///< 'edcompatible' EXTERN char *p_ead; ///< 'eadirection' EXTERN int p_emoji; ///< 'emoji' EXTERN int p_ea; ///< 'equalalways' @@ -786,9 +785,6 @@ EXTERN int p_wb; ///< 'writebackup' EXTERN OptInt p_wd; ///< 'writedelay' EXTERN int p_cdh; ///< 'cdhome' -EXTERN int p_force_on; ///< options that cannot be turned off. -EXTERN int p_force_off; ///< options that cannot be turned on. - /// "indir" values for buffer-local options. /// These need to be defined globally, so that the BV_COUNT can be used with /// b_p_script_stx[]. diff --git a/src/nvim/options.lua b/src/nvim/options.lua index c5dfa91f27..daaf73d241 100644 --- a/src/nvim/options.lua +++ b/src/nvim/options.lua @@ -6,6 +6,7 @@ --- @field varname? string --- @field pv_name? string --- @field type 'bool'|'number'|'string' +--- @field immutable? boolean --- @field list? 'comma'|'onecomma'|'commacolon'|'onecommacolon'|'flags'|'flagscomma' --- @field scope vim.option_scope[] --- @field deny_duplicates? boolean @@ -1341,7 +1342,7 @@ return { scope = { 'global' }, short_desc = N_('No description'), type = 'bool', - varname = 'p_force_off', + immutable = true, }, { abbreviation = 'cpt', @@ -2299,7 +2300,7 @@ return { scope = { 'global' }, short_desc = N_('No description'), type = 'bool', - varname = 'p_force_off', + immutable = true, }, { abbreviation = 'emo', @@ -3885,7 +3886,7 @@ return { scope = { 'global' }, short_desc = N_('No description'), type = 'bool', - varname = 'p_force_off', + immutable = true, }, { abbreviation = 'hkp', @@ -3894,7 +3895,7 @@ return { scope = { 'global' }, short_desc = N_('No description'), type = 'bool', - varname = 'p_force_off', + immutable = true, }, { abbreviation = 'hls', @@ -4292,7 +4293,7 @@ return { scope = { 'global' }, short_desc = N_('No description'), type = 'bool', - varname = 'p_force_off', + immutable = true, }, { abbreviation = 'isf', @@ -6042,7 +6043,7 @@ return { scope = { 'global' }, short_desc = N_('enable prompt in Ex mode'), type = 'bool', - varname = 'p_force_on', + immutable = true, }, { abbreviation = 'pb', @@ -6299,7 +6300,7 @@ return { scope = { 'global' }, short_desc = N_('No description'), type = 'bool', - varname = 'p_force_on', + immutable = true, }, { defaults = { if_true = 2 }, @@ -8824,7 +8825,7 @@ return { scope = { 'global' }, short_desc = N_('No description'), type = 'bool', - varname = 'p_force_off', + immutable = true, }, { abbreviation = 'tw', @@ -9069,7 +9070,7 @@ return { scope = { 'global' }, short_desc = N_('No description'), type = 'bool', - varname = 'p_force_on', + immutable = true, }, { abbreviation = 'udir',