Mirrors/fish-shell
2
0
Fork 0
mirror of https://github.com/fish-shell/fish-shell synced 2024-11-10 23:24:39 +00:00
Code Issues Projects Releases Packages Wiki Activity

Use move semantics instead of swap in env_set

Browse source 
This commit backs out certain optimizations around setting environment
variables, and replaces them with move semantics. env_set accepts a
list,  by value, permitting callers to use std::move to transfer
ownership.
This commit is contained in:
ridiculousfish 2017-08-28 02:51:34 -07:00
parent 75dd852340
commit 4baada25b9
10 changed files with 59 additions and 119 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
src/builtin_argparse.cpp
View file

@ -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

2
src/builtin_math.cpp
View file

@ -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);

2
src/builtin_set.cpp
View file

@ -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++) {

73
src/env.cpp
View file

@ -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.

19
src/env.h
View file

@ -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);

13
src/env_universal_common.cpp
View file

@ -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);

4
src/env_universal_common.h
View file

@ -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);

58
src/fish_tests.cpp
View file

@ -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"");

2
src/function.cpp
View file

@ -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
}
}

2
src/path.cpp
View file

@ -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;
}