From b51edcfcace4d26b076e95ecfee23945651b7a5c Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Tue, 12 Nov 2019 09:53:10 -0800 Subject: [PATCH] Simplify function_info_t and function_data_t Work towards cleaning up function definition. Migrate inherit_vars into props and capture their values at the point of definition. --- src/builtin_function.cpp | 23 ++++++++++++----- src/builtin_functions.cpp | 8 ++---- src/exec.cpp | 5 ++-- src/function.cpp | 54 +++++++++------------------------------ src/function.h | 17 ++++++------ 5 files changed, 40 insertions(+), 67 deletions(-) diff --git a/src/builtin_function.cpp b/src/builtin_function.cpp index 415778880..70b2328e5 100644 --- a/src/builtin_function.cpp +++ b/src/builtin_function.cpp @@ -253,17 +253,26 @@ int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_lis } // We have what we need to actually define the function. + auto props = std::make_shared(); + props->shadow_scope = opts.shadow_scope; + props->named_arguments = std::move(opts.named_arguments); + + // Populate inherit_vars. + for (const wcstring &name : opts.inherit_vars) { + if (auto var = parser.vars().get(name)) { + props->inherit_vars[name] = var->as_list(); + } + } + + props->parsed_source = source; + props->body_node = body; + function_data_t d; d.name = function_name; - if (!opts.description.empty()) d.description = opts.description; - // d.description = opts.description; + d.description = opts.description; + d.props = props; d.events = std::move(opts.events); - d.props.shadow_scope = opts.shadow_scope; - d.props.named_arguments = std::move(opts.named_arguments); - d.inherit_vars = std::move(opts.inherit_vars); - d.props.parsed_source = source; - d.props.body_node = body; function_add(std::move(d), parser); // Handle wrap targets by creating the appropriate completions. diff --git a/src/builtin_functions.cpp b/src/builtin_functions.cpp index bca9eace2..8e5cca2d0 100644 --- a/src/builtin_functions.cpp +++ b/src/builtin_functions.cpp @@ -213,15 +213,11 @@ static wcstring functions_def(const wcstring &name) { } // Output any inherited variables as `set -l` lines. - std::map inherit_vars = function_get_inherit_vars(name); - for (const auto &kv : inherit_vars) { - wcstring_list_t lst; - kv.second.to_list(lst); - + for (const auto &kv : props->inherit_vars) { // We don't know what indentation style the function uses, // so we do what fish_indent would. append_format(out, L"\n set -l %ls", kv.first.c_str()); - for (const auto &arg : lst) { + for (const auto &arg : kv.second) { wcstring earg = escape_string(arg, ESCAPE_ALL); out.push_back(L' '); out.append(earg); diff --git a/src/exec.cpp b/src/exec.cpp index 25ace6366..9e952c1c2 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -799,9 +799,8 @@ static block_t *function_prepare_environment(parser_t &parser, const process_t * idx++; } - std::map inherit_vars = function_get_inherit_vars(func_name); - for (const auto &kv : inherit_vars) { - vars.set(kv.first, ENV_LOCAL | ENV_USER, kv.second.as_list()); + for (const auto &kv : props.inherit_vars) { + vars.set(kv.first, ENV_LOCAL | ENV_USER, kv.second); } vars.set_argv(std::move(argv)); diff --git a/src/function.cpp b/src/function.cpp index 21135cdb1..2767bbc6a 100644 --- a/src/function.cpp +++ b/src/function.cpp @@ -34,23 +34,16 @@ class function_info_t { public: /// Immutable properties of the function. - std::shared_ptr props; + function_properties_ref_t props; /// Function description. This may be changed after the function is created. wcstring description; /// File where this function was defined (intern'd string). const wchar_t *const definition_file; - /// Mapping of all variables that were inherited from the function definition scope to their - /// values. - const std::map inherit_vars; /// Flag for specifying that this function was automatically loaded. const bool is_autoload; - /// Constructs relevant information from the function_data. - function_info_t(function_data_t data, const environment_t &vars, const wchar_t *filename, + function_info_t(function_properties_ref_t props, wcstring desc, const wchar_t *def_file, bool autoload); - - /// Used by function_copy. - function_info_t(const function_info_t &data, const wchar_t *filename, bool autoload); }; /// Type wrapping up the set of all functions. @@ -148,30 +141,11 @@ static void autoload_names(std::unordered_set &names, int get_hidden) } } -static std::map snapshot_vars(const wcstring_list_t &vars, - const environment_t &src) { - std::map result; - for (const wcstring &name : vars) { - auto var = src.get(name); - if (var) result[name] = std::move(*var); - } - return result; -} - -function_info_t::function_info_t(function_data_t data, const environment_t &vars, - const wchar_t *filename, bool autoload) - : props(std::make_shared(std::move(data.props))), - description(std::move(data.description)), - definition_file(intern(filename)), - inherit_vars(snapshot_vars(data.inherit_vars, vars)), - is_autoload(autoload) {} - -function_info_t::function_info_t(const function_info_t &data, const wchar_t *filename, - bool autoload) - : props(data.props), - description(data.description), - definition_file(intern(filename)), - inherit_vars(data.inherit_vars), +function_info_t::function_info_t(function_properties_ref_t props, wcstring desc, + const wchar_t *def_file, bool autoload) + : props(std::move(props)), + description(std::move(desc)), + definition_file(intern(def_file)), is_autoload(autoload) {} void function_add(const function_data_t &data, const parser_t &parser) { @@ -191,8 +165,8 @@ void function_add(const function_data_t &data, const parser_t &parser) { // Create and store a new function. const wchar_t *filename = parser.libdata().current_filename; - auto ins = funcset->funcs.emplace(data.name, - function_info_t(data, parser.vars(), filename, is_autoload)); + auto ins = funcset->funcs.emplace( + data.name, function_info_t(data.props, data.description, filename, is_autoload)); assert(ins.second && "Function should not already be present in the table"); (void)ins; @@ -261,12 +235,6 @@ bool function_get_definition(const wcstring &name, wcstring &out_definition) { return false; } -std::map function_get_inherit_vars(const wcstring &name) { - const auto funcset = function_set.acquire(); - const function_info_t *func = funcset->get_info(name); - return func ? func->inherit_vars : std::map(); -} - bool function_get_desc(const wcstring &name, wcstring &out_desc) { const auto funcset = function_set.acquire(); const function_info_t *func = funcset->get_info(name); @@ -294,12 +262,14 @@ bool function_copy(const wcstring &name, const wcstring &new_name) { // No such function. return false; } + const function_info_t &src_func = iter->second; // This new instance of the function shouldn't be tied to the definition file of the // original, so pass NULL filename, etc. // Note this will NOT overwrite an existing function with the new name. // TODO: rationalize if this behavior is desired. - funcset->funcs.emplace(new_name, function_info_t(iter->second, nullptr, false)); + funcset->funcs.emplace(new_name, + function_info_t(src_func.props, src_func.description, nullptr, false)); return true; } diff --git a/src/function.h b/src/function.h index 62f2d6a5f..27170d1e7 100644 --- a/src/function.h +++ b/src/function.h @@ -26,10 +26,16 @@ struct function_properties_t { /// List of all named arguments for this function. wcstring_list_t named_arguments; + /// Mapping of all variables that were inherited from the function definition scope to their + /// values. + std::map inherit_vars; + /// Set to true if invoking this function shadows the variables of the underlying function. bool shadow_scope{true}; }; +using function_properties_ref_t = std::shared_ptr; + /// Structure describing a function. This is used by the parser to store data on a function while /// parsing it. It is not used internally to store functions, the function_info_t structure /// is used for that purpose. Parhaps this should be merged with function_info_t. @@ -38,11 +44,8 @@ struct function_data_t { wcstring name; /// Description of function. wcstring description; - /// List of all variables that are inherited from the function definition scope. The variable - /// values are snapshotted when function_add() is called. - wcstring_list_t inherit_vars; /// Function's metadata fields - function_properties_t props; + function_properties_ref_t props; /// List of all event handlers for this function. std::vector events; }; @@ -54,7 +57,7 @@ void function_add(const function_data_t &data, const parser_t &parser); void function_remove(const wcstring &name); /// Returns the properties for a function, or nullptr if none. This does not trigger autoloading. -std::shared_ptr function_get_properties(const wcstring &name); +function_properties_ref_t function_get_properties(const wcstring &name); /// Returns by reference the definition of the function with the name \c name. Returns true if /// successful, false if no function with the given name exists. @@ -100,10 +103,6 @@ const wchar_t *function_get_definition_file(const wcstring &name); /// This does not trigger autoloading. int function_get_definition_lineno(const wcstring &name); -/// Returns a mapping of all variables of the specified function that were inherited from the scope -/// of the function definition to their values. -std::map function_get_inherit_vars(const wcstring &name); - /// Creates a new function using the same definition as the specified function. Returns true if copy /// is successful. bool function_copy(const wcstring &name, const wcstring &new_name);