From 5786f9e5c38cd06ec76b368caab749f62713ad38 Mon Sep 17 00:00:00 2001 From: Kurtis Rader Date: Sun, 13 Aug 2017 14:58:00 -0700 Subject: [PATCH] Revert "replace `var_entry_t` with `env_var_t`" This reverts commit 1c9370dbd2a3a0135c9c5fdd6b2a690dcd0c8027. --- src/builtin_set.cpp | 2 +- src/env.cpp | 120 +++++++++++++++++++---------------- src/env.h | 25 ++++---- src/env_universal_common.cpp | 42 ++++++------ src/env_universal_common.h | 4 +- src/fish_tests.cpp | 7 +- src/highlight.cpp | 4 +- src/path.cpp | 8 ++- tests/read.err | 3 - tests/read.in | 28 ++++---- tests/read.out | 3 - 11 files changed, 126 insertions(+), 120 deletions(-) diff --git a/src/builtin_set.cpp b/src/builtin_set.cpp index 810451033..cad1034ae 100644 --- a/src/builtin_set.cpp +++ b/src/builtin_set.cpp @@ -296,7 +296,7 @@ static int my_env_set(const wchar_t *cmd, const wchar_t *key, const wcstring_lis // We don't check `val->empty()` because an array var with a single empty string will be // "empty". A truly empty array var is set to the special value `ENV_NULL`. auto val = list_to_array_val(list); - retval = env_set(key, val->c_str(), scope | ENV_USER); + retval = env_set(key, *val == ENV_NULL ? NULL : val->c_str(), scope | ENV_USER); switch (retval) { case ENV_OK: { retval = STATUS_CMD_OK; diff --git a/src/env.cpp b/src/env.cpp index 63389d4ed..736a73433 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -126,8 +126,8 @@ class env_node_t { /// Pointer to next level. std::unique_ptr next; - /// Returns the given entry if present else env_var_t::missing_var. - const env_var_t find_entry(const wcstring &key); + /// Returns a pointer to the given entry if present, or NULL. + const var_entry_t *find_entry(const wcstring &key); }; class variable_entry_t { @@ -174,21 +174,27 @@ struct var_stack_t { env_node_t *next_scope_to_search(env_node_t *node); const env_node_t *next_scope_to_search(const env_node_t *node) const; - // Returns the scope used for unspecified scopes. An unspecified scope is either the topmost - // shadowing scope, or the global scope if none. This implements the default behavior of `set`. + // Returns the scope used for unspecified scopes + // An unspecified scope is either the topmost shadowing scope, or the global scope if none + // This implements the default behavior of 'set' env_node_t *resolve_unspecified_scope(); }; void var_stack_t::push(bool new_scope) { std::unique_ptr node(new env_node_t(new_scope)); - // Copy local-exported variables. + // Copy local-exported variables auto top_node = top.get(); - // Only if we introduce a new shadowing scope; i.e. not if it's just `begin; end` or - // "--no-scope-shadowing". - if (new_scope && top_node != this->global_env) { - for (auto &var : top_node->env) { - if (var.second.exportv) node->env.insert(var); + // Only if we introduce a new shadowing scope + // i.e. not if it's just `begin; end` or "--no-scope-shadowing". + if (new_scope) { + if (!(top_node == this->global_env)) { + for (auto &var : top_node->env) { + if (var.second.exportv) { + // This should copy var + node->env.insert(var); + } + } } } @@ -234,8 +240,8 @@ void var_stack_t::pop() { assert(this->top != NULL); for (const auto &entry_pair : old_top->env) { - const env_var_t &var = entry_pair.second; - if (var.exportv) { + const var_entry_t &entry = entry_pair.second; + if (entry.exportv) { this->mark_changed_exported(); break; } @@ -303,10 +309,13 @@ static bool is_electric(const wcstring &key) { return env_electric.find(key.c_str()) != env_electric.end(); } -const env_var_t env_node_t::find_entry(const wcstring &key) { - var_table_t::const_iterator entry = env.find(key); - if (entry != env.end()) return entry->second; - return env_var_t::missing_var(); +const var_entry_t *env_node_t::find_entry(const wcstring &key) { + const var_entry_t *result = NULL; + var_table_t::const_iterator where = env.find(key); + if (where != env.end()) { + result = &where->second; + } + return result; } /// Return the current umask value. @@ -950,8 +959,10 @@ void env_init(const struct config_paths_t *paths /* or NULL */) { static env_node_t *env_get_node(const wcstring &key) { env_node_t *env = vars_stack().top.get(); while (env != NULL) { - env_var_t var = env->find_entry(key); - if (!var.missing()) break; + if (env->find_entry(key) != NULL) { + break; + } + env = vars_stack().next_scope_to_search(env); } return env; @@ -1048,8 +1059,8 @@ int env_set(const wcstring &key, const wchar_t *val, env_mode_flags_t var_mode) if (preexisting_node != NULL) { var_table_t::const_iterator result = preexisting_node->env.find(key); assert(result != preexisting_node->env.end()); - const env_var_t &var = result->second; - if (var.exportv) { + const var_entry_t &entry = result->second; + if (entry.exportv) { preexisting_entry_exportv = true; has_changed_new = true; } @@ -1063,7 +1074,7 @@ int env_set(const wcstring &key, const wchar_t *val, env_mode_flags_t var_mode) } else if (preexisting_node != NULL) { node = preexisting_node; if ((var_mode & (ENV_EXPORT | ENV_UNEXPORT)) == 0) { - // Use existing entry's exportv status. + // use existing entry's exportv var_mode = //!OCLINT(parameter reassignment) preexisting_entry_exportv ? ENV_EXPORT : 0; } @@ -1097,21 +1108,20 @@ int env_set(const wcstring &key, const wchar_t *val, env_mode_flags_t var_mode) if (!done) { // Set the entry in the node. Note that operator[] accesses the existing entry, or // creates a new one. - env_var_t &var = node->env[key]; - if (var.exportv) { + var_entry_t &entry = node->env[key]; + if (entry.exportv) { // This variable already existed, and was exported. has_changed_new = true; } - - var.set_val(val); + entry.val = val; if (var_mode & ENV_EXPORT) { // The new variable is exported. - var.exportv = true; + entry.exportv = true; node->exportv = true; has_changed_new = true; } else { - var.exportv = false; - // Set the node's exported when it changes something about exports + entry.exportv = false; + // Set the node's exportv when it changes something about exports // (also when it redefines a variable to not be exported). node->exportv = has_changed_old != has_changed_new; } @@ -1209,7 +1219,7 @@ int env_remove(const wcstring &key, int var_mode) { } wcstring env_var_t::as_string(void) const { - //assert(!is_missing); + assert(!is_missing); return val; } @@ -1266,15 +1276,12 @@ env_var_t env_get(const wcstring &key, env_mode_flags_t mode) { env_node_t *env = search_local ? vars_stack().top.get() : vars_stack().global_env; while (env != NULL) { - const env_var_t var = env->find_entry(key); - if (!var.missing() && (var.exportv ? search_exported : search_unexported)) { - // The following statement is wrong because ENV_NULL is supposed to mean the var is - // set but has zero elements. Therefore we should not return missing_var. However, - // If this is changed to return `var` without the special-case then unit tests fail. - // Note that tokenize_variable_array() explicitly handles a var whose string - // representation contains only ENV_NULL. - if (var.as_string() == ENV_NULL) return env_var_t::missing_var(); - return var; + const var_entry_t *entry = env->find_entry(key); + if (entry != NULL && (entry->exportv ? search_exported : search_unexported)) { + if (entry->val == ENV_NULL) { + return env_var_t::missing_var(); + } + return entry->val; } if (has_scope) { @@ -1335,8 +1342,8 @@ bool env_exist(const wchar_t *key, env_mode_flags_t mode) { var_table_t::const_iterator result = env->env.find(key); if (result != env->env.end()) { - const env_var_t var = result->second; - return var.exportv ? test_exported : test_unexported; + const var_entry_t &res = result->second; + return res.exportv ? test_exported : test_unexported; } env = vars_stack().next_scope_to_search(env); } @@ -1378,9 +1385,9 @@ static void add_key_to_string_set(const var_table_t &envs, std::set *s bool show_exported, bool show_unexported) { var_table_t::const_iterator iter; for (iter = envs.begin(); iter != envs.end(); ++iter) { - const env_var_t &var = iter->second; + const var_entry_t &e = iter->second; - if ((var.exportv && show_exported) || (!var.exportv && show_unexported)) { + if ((e.exportv && show_exported) || (!e.exportv && show_unexported)) { // Insert this key. str_set->insert(iter->first); } @@ -1433,39 +1440,39 @@ wcstring_list_t env_get_names(int flags) { } /// Get list of all exported variables. -static void get_exported(const env_node_t *n, var_table_t &h) { +static void get_exported(const env_node_t *n, std::map *h) { if (!n) return; - if (n->new_scope) { + if (n->new_scope) get_exported(vars_stack().global_env, h); - } else { + else get_exported(n->next.get(), h); - } var_table_t::const_iterator iter; for (iter = n->env.begin(); iter != n->env.end(); ++iter) { const wcstring &key = iter->first; - const env_var_t var = iter->second; + const var_entry_t &val_entry = iter->second; - if (!var.missing() && var.exportv) { + if (val_entry.exportv && val_entry.val != ENV_NULL) { // Export the variable. Don't use std::map::insert here, since we need to overwrite // existing values from previous scopes. - h[key] = var; + (*h)[key] = val_entry.val; } else { // We need to erase from the map if we are not exporting, since a lower scope may have // exported. See #2132. - h.erase(key); + h->erase(key); } } } // Given a map from key to value, add values to out of the form key=value. -static void export_func(const var_table_t &envs, std::vector &out) { +static void export_func(const std::map &envs, std::vector &out) { out.reserve(out.size() + envs.size()); - for (auto iter = envs.begin(); iter != envs.end(); ++iter) { + std::map::const_iterator iter; + for (iter = envs.begin(); iter != envs.end(); ++iter) { const wcstring &key = iter->first; const std::string &ks = wcs2string(key); - std::string vs = wcs2string(iter->second.as_string()); + std::string vs = wcs2string(iter->second); // Arrays in the value are ASCII record separator (0x1e) delimited. But some variables // should have colons. Add those. @@ -1490,10 +1497,11 @@ void var_stack_t::update_export_array_if_necessary() { if (!this->has_changed_exported) { return; } + std::map vals; debug(4, L"env_export_arr() recalc"); - var_table_t vals; - get_exported(this->top.get(), vals); + + get_exported(this->top.get(), &vals); if (uvars()) { const wcstring_list_t uni = uvars()->get_names(true, false); @@ -1536,7 +1544,7 @@ void env_set_argv(const wchar_t *const *argv) { env_set(L"argv", sb.c_str(), ENV_LOCAL); } else { - env_set(L"argv", NULL, ENV_LOCAL); + env_set(L"argv", 0, ENV_LOCAL); } } diff --git a/src/env.h b/src/env.h index aee366f6c..b5c8131c5 100644 --- a/src/env.h +++ b/src/env.h @@ -75,19 +75,16 @@ class env_var_t { bool is_missing; public: - bool exportv; // whether the variable should be exported - static env_var_t missing_var() { env_var_t result((wcstring())); result.is_missing = true; - result.exportv = false; return result; } - env_var_t(const env_var_t &x) : val(x.val), is_missing(x.is_missing), exportv(x.exportv) {} - env_var_t(const wcstring &x) : val(x), is_missing(false), exportv(false) {} - env_var_t(const wchar_t *x) : val(x), is_missing(false), exportv(false) {} - env_var_t() : val(L""), is_missing(false), exportv(false) {} + env_var_t(const env_var_t &x) : val(x.val), is_missing(x.is_missing) {} + env_var_t(const wcstring &x) : val(x), is_missing(false) {} + env_var_t(const wchar_t *x) : val(x), is_missing(false) {} + env_var_t() : val(L""), is_missing(false) {} bool empty(void) const { return val.empty(); }; bool missing(void) const { return is_missing; } @@ -99,14 +96,10 @@ class env_var_t { env_var_t &operator=(const env_var_t &v) { is_missing = v.is_missing; - exportv = v.exportv; val = v.val; return *this; } - void set_val(const wcstring &s) { val = s; is_missing = false; } - void set_val(const wchar_t *s) { val = s; is_missing = false; } - bool operator==(const env_var_t &s) const { return is_missing == s.is_missing && val == s.val; } bool operator==(const wcstring &s) const { return !is_missing && val == s; } @@ -199,7 +192,15 @@ class env_vars_snapshot_t { extern int g_fork_count; extern bool g_use_posix_spawn; -typedef std::map var_table_t; +/// A variable entry. Stores the value of a variable and whether it should be exported. +struct var_entry_t { + wcstring val; // the value of the variable + bool exportv; // whether the variable should be exported + + var_entry_t() : exportv(false) {} +}; + +typedef std::map var_table_t; extern bool term_has_xn; // does the terminal have the "eat_newline_glitch" diff --git a/src/env_universal_common.cpp b/src/env_universal_common.cpp index 8284753c3..eac8b1e10 100644 --- a/src/env_universal_common.cpp +++ b/src/env_universal_common.cpp @@ -273,7 +273,7 @@ env_var_t env_universal_t::get(const wcstring &name) const { env_var_t result = env_var_t::missing_var(); var_table_t::const_iterator where = vars.find(name); if (where != vars.end()) { - result = where->second; + result = env_var_t(where->second.val); } return result; } @@ -295,10 +295,10 @@ void env_universal_t::set_internal(const wcstring &key, const wcstring &val, boo return; } - env_var_t &entry = vars[key]; - if (entry.exportv != exportv || entry.as_string() != val) { - entry.set_val(val); - entry.exportv = exportv; + var_entry_t *entry = &vars[key]; + if (entry->exportv != exportv || entry->val != val) { + entry->val = val; + entry->exportv = exportv; // If we are overwriting, then this is now modified. if (overwrite) { @@ -332,8 +332,8 @@ wcstring_list_t env_universal_t::get_names(bool show_exported, bool show_unexpor var_table_t::const_iterator iter; for (iter = vars.begin(); iter != vars.end(); ++iter) { const wcstring &key = iter->first; - const env_var_t &var = iter->second; - if ((var.exportv && show_exported) || (!var.exportv && show_unexported)) { + const var_entry_t &e = iter->second; + if ((e.exportv && show_exported) || (!e.exportv && show_unexported)) { result.push_back(key); } } @@ -369,18 +369,18 @@ void env_universal_t::generate_callbacks(const var_table_t &new_vars, } // See if the value has changed. - const env_var_t &new_entry = iter->second; + const var_entry_t &new_entry = iter->second; var_table_t::const_iterator existing = this->vars.find(key); if (existing == this->vars.end() || existing->second.exportv != new_entry.exportv || - existing->second != new_entry) { + existing->second.val != new_entry.val) { // Value has changed. callbacks.push_back( - callback_data_t(new_entry.exportv ? SET_EXPORT : SET, key, new_entry.as_string())); + callback_data_t(new_entry.exportv ? SET_EXPORT : SET, key, new_entry.val)); } } } -void env_universal_t::acquire_variables(var_table_t &vars_to_acquire) { +void env_universal_t::acquire_variables(var_table_t *vars_to_acquire) { // Copy modified values from existing vars to vars_to_acquire. for (std::set::iterator iter = this->modified.begin(); iter != this->modified.end(); ++iter) { @@ -388,19 +388,19 @@ void env_universal_t::acquire_variables(var_table_t &vars_to_acquire) { var_table_t::iterator src_iter = this->vars.find(key); if (src_iter == this->vars.end()) { /* The value has been deleted. */ - vars_to_acquire.erase(key); + vars_to_acquire->erase(key); } else { // The value has been modified. Copy it over. Note we can destructively modify the // source entry in vars since we are about to get rid of this->vars entirely. - env_var_t &src = src_iter->second; - env_var_t &dst = vars_to_acquire[key]; - dst.set_val(src.as_string()); + var_entry_t &src = src_iter->second; + var_entry_t &dst = (*vars_to_acquire)[key]; + dst.val = std::move(src.val); dst.exportv = src.exportv; } } // We have constructed all the callbacks and updated vars_to_acquire. Acquire it! - this->vars = std::move(vars_to_acquire); + this->vars = std::move(*vars_to_acquire); } void env_universal_t::load_from_fd(int fd, callback_data_list_t &callbacks) { @@ -418,7 +418,7 @@ void env_universal_t::load_from_fd(int fd, callback_data_list_t &callbacks) { this->generate_callbacks(new_vars, callbacks); // Acquire the new variables. - this->acquire_variables(new_vars); + this->acquire_variables(&new_vars); last_read_file = current_file; } } @@ -464,8 +464,8 @@ bool env_universal_t::write_to_fd(int fd, const wcstring &path) { // Append the entry. Note that append_file_entry may fail, but that only affects one // variable; soldier on. const wcstring &key = iter->first; - const env_var_t &var = iter->second; - append_file_entry(var.exportv ? SET_EXPORT : SET, key, var.as_string(), &contents, &storage); + const var_entry_t &entry = iter->second; + append_file_entry(entry.exportv ? SET_EXPORT : SET, key, entry.val, &contents, &storage); // Go to next. ++iter; @@ -845,9 +845,9 @@ void env_universal_t::parse_message_internal(const wcstring &msgstr, var_table_t wcstring val; if (unescape_string(tmp + 1, &val, 0)) { - env_var_t &entry = (*vars)[key]; + var_entry_t &entry = (*vars)[key]; entry.exportv = exportv; - entry.set_val(val); // acquire the value + entry.val = std::move(val); // acquire the value } } else { debug(1, PARSE_ERR, msg); diff --git a/src/env_universal_common.h b/src/env_universal_common.h index bca672ac7..a73a57c43 100644 --- a/src/env_universal_common.h +++ b/src/env_universal_common.h @@ -60,9 +60,9 @@ class env_universal_t { // the new vars. void generate_callbacks(const var_table_t &new_vars, callback_data_list_t &callbacks) const; - // Given a variable table, copy unmodified values into self. May destructively modify + // Given a variable table, copy unmodified values into self. May destructively modified // vars_to_acquire. - void acquire_variables(var_table_t &vars_to_acquire); + void acquire_variables(var_table_t *vars_to_acquire); static void parse_message_internal(const wcstring &msg, var_table_t *vars, wcstring *storage); static var_table_t read_message_internal(int fd); diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 9c539d96e..09e57e091 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -215,7 +215,6 @@ static void popd() { /// Test that the fish functions for converting strings to numbers work. static void test_str_to_num() { - say(L"Testing str_to_num"); const wchar_t *end; int i; long l; @@ -4369,9 +4368,6 @@ int main(int argc, char **argv) { // Set default signal handlers, so we can ctrl-C out of this. signal_reset_handlers(); - if (should_test_function("utility_functions")) test_utility_functions(); - if (should_test_function("wcstring_tok")) test_wcstring_tok(); - if (should_test_function("env_vars")) test_env_vars(); if (should_test_function("str_to_num")) test_str_to_num(); if (should_test_function("highlighting")) test_highlighting(); if (should_test_function("new_parser_ll2")) test_new_parser_ll2(); @@ -4413,12 +4409,15 @@ int main(int argc, char **argv) { if (should_test_function("autosuggestion_ignores")) test_autosuggestion_ignores(); if (should_test_function("autosuggestion_combining")) test_autosuggestion_combining(); if (should_test_function("autosuggest_suggest_special")) test_autosuggest_suggest_special(); + if (should_test_function("wcstring_tok")) test_wcstring_tok(); if (should_test_function("history")) history_tests_t::test_history(); if (should_test_function("history_merge")) history_tests_t::test_history_merge(); if (should_test_function("history_races")) history_tests_t::test_history_races(); if (should_test_function("history_formats")) history_tests_t::test_history_formats(); if (should_test_function("string")) test_string(); + if (should_test_function("env_vars")) test_env_vars(); if (should_test_function("illegal_command_exit_code")) test_illegal_command_exit_code(); + if (should_test_function("utility_functions")) test_utility_functions(); // history_tests_t::test_history_speed(); say(L"Encountered %d errors in low-level tests", err_count); diff --git a/src/highlight.cpp b/src/highlight.cpp index aefafa436..2a64336f9 100644 --- a/src/highlight.cpp +++ b/src/highlight.cpp @@ -279,6 +279,7 @@ rgb_color_t highlight_get_color(highlight_spec_t highlight, bool is_background) // Handle modifiers. if (highlight & highlight_modifier_valid_path) { env_var_t var2 = env_get(L"fish_color_valid_path"); + const wcstring val2 = var2.missing() ? L"" : var2.c_str(); rgb_color_t result2 = parse_color(var2, is_background); if (result.is_normal()) @@ -1013,9 +1014,8 @@ static bool command_is_valid(const wcstring &cmd, enum parse_statement_decoratio if (!is_valid && command_ok) is_valid = path_get_path(cmd, NULL, vars); // Implicit cd - if (!is_valid && implicit_cd_ok) { + if (!is_valid && implicit_cd_ok) is_valid = path_can_be_implicit_cd(cmd, NULL, working_directory.c_str(), vars); - } // Return what we got. return is_valid; diff --git a/src/path.cpp b/src/path.cpp index 70391067d..40ecd0757 100644 --- a/src/path.cpp +++ b/src/path.cpp @@ -48,18 +48,20 @@ static bool path_get_path_core(const wcstring &cmd, wcstring *out_path, } int err = ENOENT; - wcstring_list_t pathsv; + wcstring bin_path; if (!bin_path_var.missing()) { - bin_path_var.to_list(pathsv); + bin_path = bin_path_var.as_string(); } else { // Note that PREFIX is defined in the `Makefile` and is thus defined when this module is // compiled. This ensures we always default to "/bin", "/usr/bin" and the bin dir defined // for the fish programs. Possibly with a duplicate dir if PREFIX is empty, "/", "/usr" or // "/usr/". If the PREFIX duplicates /bin or /usr/bin that is harmless other than a trivial // amount of time testing a path we've already tested. - pathsv = wcstring_list_t({L"/bin", L"/usr/bin", PREFIX L"/bin"}); + bin_path = *list_to_array_val(wcstring_list_t({L"/bin", L"/usr/bin", PREFIX L"/bin"})); } + wcstring_list_t pathsv; + bin_path_var.to_list(pathsv); for (auto next_path : pathsv) { if (next_path.empty()) continue; append_path_component(next_path, cmd); diff --git a/tests/read.err b/tests/read.err index d76028bc0..676c6e2d1 100644 --- a/tests/read.err +++ b/tests/read.err @@ -8,9 +8,6 @@ read: Expected at least 1 args, got only 0 read: Expected 1 args, got 0 read: Expected 1 args, got 2 -#################### -# Verify correct behavior of subcommands and splitting of input. - #################### # Test splitting input diff --git a/tests/read.in b/tests/read.in index cd5bfa07e..36cbcc98e 100644 --- a/tests/read.in +++ b/tests/read.in @@ -3,6 +3,8 @@ # Test read builtin and IFS. # +########### +# Start by testing that invocation errors are handled correctly. logmsg Read with no vars is an error read @@ -11,19 +13,19 @@ read -a read -a v1 v2 read -a v1 -logmsg Verify correct behavior of subcommands and splitting of input. -begin - count (echo one\ntwo) - set -l IFS \t - count (echo one\ntwo) - set -l IFS - count (echo one\ntwo) - echo [(echo -n one\ntwo)] - count (echo one\ntwo\n) - echo [(echo -n one\ntwo\n)] - count (echo one\ntwo\n\n) - echo [(echo -n one\ntwo\n\n)] -end +########### +# Verify correct behavior of subcommands and splitting of input. +count (echo one\ntwo) +set -l IFS \t +count (echo one\ntwo) +set -l IFS +count (echo one\ntwo) +echo [(echo -n one\ntwo)] +count (echo one\ntwo\n) +echo [(echo -n one\ntwo\n)] +count (echo one\ntwo\n\n) +echo [(echo -n one\ntwo\n\n)] +set -le IFS function print_vars --no-scope-shadowing set -l space diff --git a/tests/read.out b/tests/read.out index eb1c23a6d..4b495bdc3 100644 --- a/tests/read.out +++ b/tests/read.out @@ -4,9 +4,6 @@ #################### # Read with -a and anything other than exactly on var name is an error - -#################### -# Verify correct behavior of subcommands and splitting of input. 2 2 1