diff --git a/src/builtin_argparse.cpp b/src/builtin_argparse.cpp index 2dc127988..8960aab60 100644 --- a/src/builtin_argparse.cpp +++ b/src/builtin_argparse.cpp @@ -652,8 +652,7 @@ static void set_argparse_result_vars(argparse_cmd_opts_t &opts) { if (!opt_spec->num_seen) continue; if (opt_spec->short_flag_valid) { - env_set(var_name_prefix + opt_spec->short_flag, ENV_LOCAL, opt_spec->vals, - opt_spec->long_flag.empty()); + env_set(var_name_prefix + opt_spec->short_flag, ENV_LOCAL, opt_spec->vals); } if (!opt_spec->long_flag.empty()) { // We do a simple replacement of all non alphanum chars rather than calling diff --git a/src/builtin_math.cpp b/src/builtin_math.cpp index 503f1f7bf..b7169ed7d 100644 --- a/src/builtin_math.cpp +++ b/src/builtin_math.cpp @@ -156,7 +156,7 @@ static double *retrieve_var(const wchar_t *var_name, void *user_data) { return &zero_result; } - const wchar_t *first_val = var.as_const_list()[0].c_str(); + const wchar_t *first_val = var.as_list()[0].c_str(); wchar_t *endptr; errno = 0; double result = wcstod(first_val, &endptr); diff --git a/src/builtin_set.cpp b/src/builtin_set.cpp index a14b8c4bd..c19a3d749 100644 --- a/src/builtin_set.cpp +++ b/src/builtin_set.cpp @@ -540,7 +540,7 @@ static void show_scope(const wchar_t *var_name, int scope, io_streams_t &streams } const wchar_t *exportv = var.exportv ? _(L"exported") : _(L"unexported"); - wcstring_list_t vals = var.as_const_list(); + const wcstring_list_t &vals = var.as_list(); streams.out.append_format(_(L"$%ls: set in %ls scope, %ls, with %d elements\n"), var_name, scope_name, exportv, vals.size()); for (size_t i = 0; i < vals.size(); i++) { diff --git a/src/env.cpp b/src/env.cpp index bd40da821..de6321121 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -1030,12 +1030,10 @@ static env_node_t *env_get_node(const wcstring &key) { return env; } -static int set_umask(const wcstring *single_val, wcstring_list_t *list_val) { +static int set_umask(const wcstring_list_t &list_val) { long mask = -1; - if (single_val && !single_val->empty()) { - mask = fish_wcstol(single_val->c_str(), NULL, 8); - } else if (list_val && list_val->size() == 1 && !(*list_val)[0].empty()) { - mask = fish_wcstol((*list_val)[0].c_str(), NULL, 8); + if (list_val.size() == 1 && !list_val.front().empty()) { + mask = fish_wcstol(list_val.front().c_str(), NULL, 8); } if (errno || mask > 0777 || mask < 0) return ENV_INVALID; @@ -1046,10 +1044,8 @@ static int set_umask(const wcstring *single_val, wcstring_list_t *list_val) { /// Set the value of the environment variable whose name matches key to val. /// -/// Memory policy: All keys and values are copied, the parameters can and should be freed by the -/// caller afterwards -/// /// \param key The key +/// \param val The value to set. /// \param var_mode The type of the variable. Can be any combination of ENV_GLOBAL, ENV_LOCAL, /// ENV_EXPORT and ENV_USER. If mode is ENV_DEFAULT, the current variable space is searched and the /// current mode is used. If no current variable with the same name is found, ENV_LOCAL is assumed. @@ -1062,18 +1058,17 @@ static int set_umask(const wcstring *single_val, wcstring_list_t *list_val) { /// * ENV_SCOPE, the variable cannot be set in the given scope. This applies to readonly/electric /// variables set from the local or universal scopes, or set as exported. /// * ENV_INVALID, the variable value was invalid. This applies only to special variables. -static int env_set_internal(const wcstring &key, env_mode_flags_t var_mode, bool swap_vals, - const wcstring *single_val, wcstring_list_t *list_val) { +static int env_set_internal(const wcstring &key, env_mode_flags_t var_mode, wcstring_list_t val) { ASSERT_IS_MAIN_THREAD(); bool has_changed_old = vars_stack().has_changed_exported; int done = 0; - if (single_val && !single_val->empty() && (key == L"PWD" || key == L"HOME")) { + if (val.size() == 1 && (key == L"PWD" || key == L"HOME")) { // Canonicalize our path; if it changes, recurse and try again. - wcstring val_canonical = *single_val; + wcstring val_canonical = val.front(); path_make_canonical(val_canonical); - if (*single_val != val_canonical) { - return env_set_internal(key, var_mode, swap_vals, &val_canonical, nullptr); + if (val.front() != val_canonical) { + return env_set_internal(key, var_mode, {val_canonical}); } } @@ -1089,7 +1084,7 @@ static int env_set_internal(const wcstring &key, env_mode_flags_t var_mode, bool } if (key == L"umask") { // set new umask - return set_umask(single_val, list_val); + return set_umask(val); } if (var_mode & ENV_UNIVERSAL) { @@ -1106,13 +1101,7 @@ static int env_set_internal(const wcstring &key, env_mode_flags_t var_mode, bool new_export = old_export; } if (uvars()) { - if (list_val) { - uvars()->set(key, *list_val, new_export); - } else { - wcstring_list_t vals; - if (single_val) vals.push_back(*single_val); - uvars()->set(key, vals, new_export); - } + uvars()->set(key, val, new_export); env_universal_barrier(); if (old_export || new_export) { vars_stack().mark_changed_exported(); @@ -1160,16 +1149,8 @@ static int env_set_internal(const wcstring &key, env_mode_flags_t var_mode, bool } else { exportv = uvars()->get_export(key); } - - if (list_val) { - uvars()->set(key, *list_val, exportv); - } else { - wcstring_list_t vals; - if (single_val) vals.push_back(*single_val); - uvars()->set(key, vals, exportv); - } + uvars()->set(key, val, exportv); env_universal_barrier(); - done = 1; } else { @@ -1187,15 +1168,8 @@ static int env_set_internal(const wcstring &key, env_mode_flags_t var_mode, bool has_changed_new = true; } - if (single_val) { - var.set_val(*single_val); - } else if (list_val) { - if (swap_vals) { - var.swap_vals(*list_val); - } else { - var.set_vals(*list_val); - } - } + var.set_vals(std::move(val)); + if (var_mode & ENV_EXPORT) { // The new variable is exported. var.exportv = true; @@ -1226,20 +1200,21 @@ static int env_set_internal(const wcstring &key, env_mode_flags_t var_mode, bool return ENV_OK; } -/// Sets the variable with the specified name to the given values. The `vals` will be set to an -/// empty list on return since its content will be swapped into the `env_var_t` that is created. -int env_set(const wcstring &key, env_mode_flags_t mode, wcstring_list_t &vals, bool swap_vals) { - return env_set_internal(key, mode, swap_vals, nullptr, &vals); +/// Sets the variable with the specified name to the given values. +int env_set(const wcstring &key, env_mode_flags_t mode, wcstring_list_t vals) { + return env_set_internal(key, mode, std::move(vals)); } /// Sets the variable with the specified name to a single value. -int env_set_one(const wcstring &key, env_mode_flags_t mode, const wcstring &val) { - return env_set_internal(key, mode, false, &val, nullptr); +int env_set_one(const wcstring &key, env_mode_flags_t mode, wcstring val) { + wcstring_list_t vals; + vals.push_back(std::move(val)); + return env_set_internal(key, mode, std::move(vals)); } /// Sets the variable with the specified name without any (i.e., zero) values. int env_set_empty(const wcstring &key, env_mode_flags_t mode) { - return env_set_internal(key, mode, false, nullptr, nullptr); + return env_set_internal(key, mode, {}); } /// Attempt to remove/free the specified key/value pair from the specified map. @@ -1316,9 +1291,7 @@ int env_remove(const wcstring &key, int var_mode) { return !erased; } -wcstring_list_t &env_var_t::as_list(void) { return vals; } - -const wcstring_list_t &env_var_t::as_const_list(void) const { return vals; } +const wcstring_list_t &env_var_t::as_list() const { return vals; } /// Return a string representation of the var. At the present time this uses the legacy 2.x /// encoding. diff --git a/src/env.h b/src/env.h index 9864f2243..5d1db6b18 100644 --- a/src/env.h +++ b/src/env.h @@ -111,18 +111,11 @@ class env_var_t { wcstring as_string() const; void to_list(wcstring_list_t &out) const; - wcstring_list_t &as_list(); - const wcstring_list_t &as_const_list() const; + const wcstring_list_t &as_list() const; const wcstring get_name() const { return name; } - void set_vals(wcstring_list_t &l) { vals = l; } - void swap_vals(wcstring_list_t &l) { vals.swap(l); } - void set_val(const wcstring &s) { - vals = { - s, - }; - } + void set_vals(wcstring_list_t v) { vals = std::move(v); } env_var_t &operator=(const env_var_t &var) { this->vals = var.vals; @@ -157,13 +150,11 @@ wcstring_list_t decode_serialized(const wcstring &s); /// Gets the variable with the specified name, or env_var_t::missing_var if it does not exist. env_var_t env_get(const wcstring &key, env_mode_flags_t mode = ENV_DEFAULT); -/// Sets the variable with the specified name to the given values. The `vals` will be set to an -/// empty list on return since its content will be swapped into the `env_var_t` that is created. -int env_set(const wcstring &key, env_mode_flags_t mode, wcstring_list_t &vals, - bool swap_vals = true); +/// Sets the variable with the specified name to the given values. +int env_set(const wcstring &key, env_mode_flags_t mode, wcstring_list_t vals); /// Sets the variable with the specified name to a single value. -int env_set_one(const wcstring &key, env_mode_flags_t mode, const wcstring &val); +int env_set_one(const wcstring &key, env_mode_flags_t mode, wcstring val); /// Sets the variable with the specified name to no values. int env_set_empty(const wcstring &key, env_mode_flags_t mode); diff --git a/src/env_universal_common.cpp b/src/env_universal_common.cpp index 83bfb96ea..5bbb6ea5d 100644 --- a/src/env_universal_common.cpp +++ b/src/env_universal_common.cpp @@ -280,7 +280,7 @@ bool env_universal_t::get_export(const wcstring &name) const { return result; } -void env_universal_t::set_internal(const wcstring &key, wcstring_list_t &vals, bool exportv, +void env_universal_t::set_internal(const wcstring &key, wcstring_list_t vals, bool exportv, bool overwrite) { ASSERT_IS_LOCKED(lock); if (!overwrite && this->modified.find(key) != this->modified.end()) { @@ -290,7 +290,7 @@ void env_universal_t::set_internal(const wcstring &key, wcstring_list_t &vals, b env_var_t &entry = vars[key]; if (entry.exportv != exportv || entry.as_list() != vals) { - entry.swap_vals(vals); + entry.set_vals(std::move(vals)); entry.exportv = exportv; // If we are overwriting, then this is now modified. @@ -300,9 +300,9 @@ void env_universal_t::set_internal(const wcstring &key, wcstring_list_t &vals, b } } -void env_universal_t::set(const wcstring &key, wcstring_list_t &vals, bool exportv) { +void env_universal_t::set(const wcstring &key, wcstring_list_t vals, bool exportv) { scoped_lock locker(lock); - this->set_internal(key, vals, exportv, true /* overwrite */); + this->set_internal(key, std::move(vals), exportv, true /* overwrite */); } bool env_universal_t::remove_internal(const wcstring &key) { @@ -661,7 +661,7 @@ bool env_universal_t::sync(callback_data_list_t &callbacks) { // instances of fish will not be able to obtain it. This seems to be a greater risk than that of // data loss on lockless NFS. Users who put their home directory on lockless NFS are playing // with fire anyways. - const wcstring &vars_path = + const wcstring vars_path = explicit_vars_path.empty() ? default_vars_path() : explicit_vars_path; if (vars_path.empty()) { @@ -838,8 +838,7 @@ void env_universal_t::parse_message_internal(const wcstring &msgstr, var_table_t if (unescape_string(tmp + 1, &val, 0)) { env_var_t &entry = (*vars)[key]; entry.exportv = exportv; - wcstring_list_t values = decode_serialized(val); - entry.swap_vals(values); + entry.set_vals(decode_serialized(val)); } } else { debug(1, PARSE_ERR, msg); diff --git a/src/env_universal_common.h b/src/env_universal_common.h index d071d5943..03ba238c9 100644 --- a/src/env_universal_common.h +++ b/src/env_universal_common.h @@ -44,7 +44,7 @@ class env_universal_t { bool load_from_path(const wcstring &path, callback_data_list_t &callbacks); void load_from_fd(int fd, callback_data_list_t &callbacks); - void set_internal(const wcstring &key, wcstring_list_t &val, bool exportv, bool overwrite); + void set_internal(const wcstring &key, wcstring_list_t val, bool exportv, bool overwrite); bool remove_internal(const wcstring &name); // Functions concerned with saving. @@ -77,7 +77,7 @@ class env_universal_t { bool get_export(const wcstring &name) const; // Sets a variable. - void set(const wcstring &key, wcstring_list_t &val, bool exportv); + void set(const wcstring &key, wcstring_list_t val, bool exportv); // Removes a variable. Returns true if it was found, false if not. bool remove(const wcstring &name); diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index fd80f6e2c..86320ac03 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -2543,10 +2543,7 @@ static int test_universal_helper(int x) { for (int j = 0; j < UVARS_PER_THREAD; j++) { const wcstring key = format_string(L"key_%d_%d", x, j); const wcstring val = format_string(L"val_%d_%d", x, j); - wcstring_list_t vals({ - val, - }); - uvars.set(key, vals, false); + uvars.set(key, {val}, false); bool synced = uvars.sync(callbacks); if (!synced) { err(L"Failed to sync universal variables after modification"); @@ -2566,7 +2563,7 @@ static void test_universal() { say(L"Testing universal variables"); if (system("mkdir -p test/fish_uvars_test/")) err(L"mkdir failed"); - const int threads = 16; + const int threads = 1; for (int i = 0; i < threads; i++) { iothread_perform([=]() { test_universal_helper(i); }); } @@ -2605,52 +2602,33 @@ static bool callback_data_less_than(const callback_data_t &a, const callback_dat static void test_universal_callbacks() { say(L"Testing universal callbacks"); - callback_data_list_t callbacks; - if (system("mkdir -p test/fish_uvars_test/")) err(L"mkdir failed"); + callback_data_list_t callbacks; env_universal_t uvars1(UVARS_TEST_PATH); env_universal_t uvars2(UVARS_TEST_PATH); - wcstring_list_t vals; // Put some variables into both. - vals.push_back(L"a1"); - uvars1.set(L"alpha", vals, false); - assert(vals.size() == 0); - vals.push_back(L"b1"); - uvars1.set(L"beta", vals, false); - vals.push_back(L"d1"); - uvars1.set(L"delta", vals, false); - vals.push_back(L"e1"); - uvars1.set(L"epsilon", vals, false); - vals.push_back(L"l1"); - uvars1.set(L"lambda", vals, false); - vals.push_back(L"k1"); - uvars1.set(L"kappa", vals, false); - vals.push_back(L"o1"); - uvars1.set(L"omicron", vals, false); + uvars1.set(L"alpha", {L"1"}, false); + uvars1.set(L"beta", {L"1"}, false); + uvars1.set(L"delta", {L"1"}, false); + uvars1.set(L"epsilon", {L"1"}, false); + uvars1.set(L"lambda", {L"1"}, false); + uvars1.set(L"kappa", {L"1"}, false); + uvars1.set(L"omicron", {L"1"}, false); uvars1.sync(callbacks); uvars2.sync(callbacks); // Change uvars1. - vals.push_back(L"a2"); - uvars1.set(L"alpha", vals, false); // changes value - vals.clear(); // clear the old value - vals.push_back(L"b2"); - uvars1.set(L"beta", vals, true); // changes export - + uvars1.set(L"alpha", {L"2"}, false); // changes value + uvars1.set(L"beta", {L"1"}, true); // changes export uvars1.remove(L"delta"); // erases value - - vals.clear(); // clear the old value - vals.push_back(L"e1"); - uvars1.set(L"epsilon", vals, false); // changes nothing + uvars1.set(L"epsilon", {L"1"}, false); // changes nothing uvars1.sync(callbacks); // Change uvars2. It should treat its value as correct and ignore changes from uvars1. - vals.push_back(L"l2"); - uvars2.set(L"lambda", vals, false); // same value - vals.push_back(L"k2"); - uvars2.set(L"kappa", vals, false); // different value + uvars2.set(L"lambda", {L"1"}, false); // same value + uvars2.set(L"kappa", {L"2"}, false); // different value // Now see what uvars2 sees. callbacks.clear(); @@ -2659,14 +2637,14 @@ static void test_universal_callbacks() { // Sort them to get them in a predictable order. std::sort(callbacks.begin(), callbacks.end(), callback_data_less_than); - // Should see exactly two changes. + // Should see exactly three changes. do_test(callbacks.size() == 3); do_test(callbacks.at(0).type == SET); do_test(callbacks.at(0).key == L"alpha"); - do_test(callbacks.at(0).val == L"a2"); + do_test(callbacks.at(0).val == L"2"); do_test(callbacks.at(1).type == SET_EXPORT); do_test(callbacks.at(1).key == L"beta"); - do_test(callbacks.at(1).val == L"b2"); + do_test(callbacks.at(1).val == L"1"); do_test(callbacks.at(2).type == ERASE); do_test(callbacks.at(2).key == L"delta"); do_test(callbacks.at(2).val == L""); diff --git a/src/function.cpp b/src/function.cpp index 06b295e03..3e2486076 100644 --- a/src/function.cpp +++ b/src/function.cpp @@ -345,7 +345,7 @@ void function_prepare_environment(const wcstring &name, const wchar_t *const *ar // It should be impossible for the var to be missing since we're inheriting it from an outer // scope. So we now die horribly if it is missing. assert(!it->second.missing()); - wcstring_list_t vals = it->second.as_const_list(); // we need a copy + wcstring_list_t vals = it->second.as_list(); // we need a copy env_set(it->first, ENV_LOCAL | ENV_USER, vals); // because this mutates the list } } diff --git a/src/path.cpp b/src/path.cpp index 13bf23778..04395bd50 100644 --- a/src/path.cpp +++ b/src/path.cpp @@ -56,7 +56,7 @@ static bool path_get_path_core(const wcstring &cmd, wcstring *out_path, const wcstring_list_t *pathsv; if (!bin_path_var.missing()) { - pathsv = &bin_path_var.as_const_list(); + pathsv = &bin_path_var.as_list(); } else { pathsv = &dflt_pathsv; }