From de23ce6ac1ea997dd7456bba0ef0a821dd690929 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Fri, 9 Feb 2018 21:53:06 -0800 Subject: [PATCH 01/14] Functions to store nodes Prior to this fix, functions stored a string representation of their contents. Switch them to storing a parsed source reference and the tnode of the contents. This is part of an effort to avoid reparsing a function's contents every time it executes. --- src/builtin_function.cpp | 8 +++++--- src/builtin_function.h | 3 ++- src/builtin_functions.cpp | 2 +- src/fish_tests.cpp | 3 ++- src/function.cpp | 40 ++++++++++++++++++++++++--------------- src/function.h | 30 ++++++++++++++--------------- src/parse_execution.cpp | 29 +++------------------------- src/parse_execution.h | 2 +- src/parser.cpp | 2 +- src/tnode.h | 3 +-- 10 files changed, 56 insertions(+), 66 deletions(-) diff --git a/src/builtin_function.cpp b/src/builtin_function.cpp index 9576ce9dc..e865c2a4c 100644 --- a/src/builtin_function.cpp +++ b/src/builtin_function.cpp @@ -200,7 +200,8 @@ static int validate_function_name(int argc, const wchar_t *const *argv, wcstring /// Define a function. Calls into `function.cpp` to perform the heavy lifting of defining a /// function. int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_list_t &c_args, - const wcstring &contents, int definition_line_offset) { + const parsed_source_ref_t &source, tnode_t body) { + assert(source && "Missing source in builtin_function"); // The wgetopt function expects 'function' as the first argument. Make a new wcstring_list with // that property. This is needed because this builtin has a different signature than the other // builtins. @@ -257,8 +258,9 @@ int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_lis e.function_name = d.name; } - d.definition = contents.c_str(); - function_add(d, parser, definition_line_offset); + d.parsed_source = source; + d.body_node = body; + function_add(d, parser); // Handle wrap targets by creating the appropriate completions. for (size_t w = 0; w < opts.wrap_targets.size(); w++) { diff --git a/src/builtin_function.h b/src/builtin_function.h index f144924c7..650f7254e 100644 --- a/src/builtin_function.h +++ b/src/builtin_function.h @@ -3,10 +3,11 @@ #define FISH_BUILTIN_FUNCTION_H #include "common.h" +#include "parse_tree.h" class parser_t; struct io_streams_t; int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_list_t &c_args, - const wcstring &contents, int definition_line_offset); + const parsed_source_ref_t &source, tnode_t body); #endif diff --git a/src/builtin_functions.cpp b/src/builtin_functions.cpp index a7e6ff3b4..c1142a646 100644 --- a/src/builtin_functions.cpp +++ b/src/builtin_functions.cpp @@ -227,7 +227,7 @@ static int report_function_metadata(const wchar_t *funcname, bool verbose, io_st path = function_get_definition_file(funcname); if (path) { autoloaded = function_is_autoloaded(funcname) ? L"autoloaded" : L"not-autoloaded"; - line_number = function_get_definition_offset(funcname); + line_number = function_get_definition_lineno(funcname); } else { path = L"stdin"; } diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 3ec041cba..408152640 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -2166,9 +2166,10 @@ static void test_complete(void) { do_test(completions.at(0).completion == L"space"); // Add a function and test completing it in various ways. + // Note we're depending on function_data_t not complaining when given missing parsed_source / + // body_node. struct function_data_t func_data = {}; func_data.name = L"scuttlebutt"; - func_data.definition = L"echo gongoozle"; function_add(func_data, parser_t::principal_parser()); // Complete a function name. diff --git a/src/function.cpp b/src/function.cpp index 5efadc689..eae881746 100644 --- a/src/function.cpp +++ b/src/function.cpp @@ -10,11 +10,12 @@ #include #include +#include #include #include -#include #include #include +#include #include #include "autoload.h" @@ -117,33 +118,32 @@ static std::map snapshot_vars(const wcstring_list_t &vars) } function_info_t::function_info_t(const function_data_t &data, const wchar_t *filename, - int def_offset, bool autoload) - : definition(data.definition), + bool autoload) + : parsed_source(data.parsed_source), + body_node(data.body_node), description(data.description), definition_file(intern(filename)), - definition_offset(def_offset), named_arguments(data.named_arguments), inherit_vars(snapshot_vars(data.inherit_vars)), is_autoload(autoload), shadow_scope(data.shadow_scope) {} function_info_t::function_info_t(const function_info_t &data, const wchar_t *filename, - int def_offset, bool autoload) - : definition(data.definition), + bool autoload) + : parsed_source(data.parsed_source), + body_node(data.body_node), description(data.description), definition_file(intern(filename)), - definition_offset(def_offset), named_arguments(data.named_arguments), inherit_vars(data.inherit_vars), is_autoload(autoload), shadow_scope(data.shadow_scope) {} -void function_add(const function_data_t &data, const parser_t &parser, int definition_line_offset) { +void function_add(const function_data_t &data, const parser_t &parser) { UNUSED(parser); ASSERT_IS_MAIN_THREAD(); CHECK(!data.name.empty(), ); //!OCLINT(multiple unary operator) - CHECK(data.definition, ); scoped_rlock locker(functions_lock); // Remove the old function. @@ -152,8 +152,8 @@ void function_add(const function_data_t &data, const parser_t &parser, int defin // Create and store a new function. const wchar_t *filename = reader_current_filename(); - const function_map_t::value_type new_pair( - data.name, function_info_t(data, filename, definition_line_offset, is_autoload)); + const function_map_t::value_type new_pair(data.name, + function_info_t(data, filename, is_autoload)); loaded_functions.insert(new_pair); // Add event handlers. @@ -223,7 +223,7 @@ bool function_get_definition(const wcstring &name, wcstring *out_definition) { scoped_rlock locker(functions_lock); const function_info_t *func = function_get(name); if (func && out_definition) { - out_definition->assign(func->definition); + out_definition->assign(func->body_node.get_source(func->parsed_source->src)); } return func != NULL; } @@ -275,7 +275,7 @@ bool function_copy(const wcstring &name, const wcstring &new_name) { // This new instance of the function shouldn't be tied to the definition file of the // original, so pass NULL filename, etc. const function_map_t::value_type new_pair(new_name, - function_info_t(iter->second, NULL, 0, false)); + function_info_t(iter->second, NULL, false)); loaded_functions.insert(new_pair); result = true; } @@ -311,10 +311,20 @@ bool function_is_autoloaded(const wcstring &name) { return func->is_autoload; } -int function_get_definition_offset(const wcstring &name) { +int function_get_definition_lineno(const wcstring &name) { scoped_rlock locker(functions_lock); const function_info_t *func = function_get(name); - return func ? func->definition_offset : -1; + if (!func) return -1; + // return one plus the number of newlines at offsets less than the start of our function's statement (which includes the header). + // TODO: merge with line_offset_of_character_at_offset? + auto block_stat = func->body_node.try_get_parent(); + assert(block_stat && "Function body is not part of block statement"); + auto source_range = block_stat.source_range(); + assert(source_range && "Function has no source range"); + uint32_t func_start = source_range->start; + const wcstring &source = func->parsed_source->src; + assert(func_start <= source.size() && "function start out of bounds"); + return 1 + std::count(source.begin(), source.begin() + func_start, L'\n'); } // Setup the environment for the function. There are three components of the environment: diff --git a/src/function.h b/src/function.h index 514a3e65a..6d50107b9 100644 --- a/src/function.h +++ b/src/function.h @@ -10,6 +10,8 @@ #include "common.h" #include "env.h" #include "event.h" +#include "parse_tree.h" +#include "tnode.h" class parser_t; @@ -21,8 +23,10 @@ struct function_data_t { wcstring name; /// Description of function. wcstring description; - /// Function definition. - const wchar_t *definition; + /// Parsed source containing the function. + parsed_source_ref_t parsed_source; + /// Node containing the function body, pointing into parsed_source. + tnode_t body_node; /// List of all event handlers for this function. std::vector events; /// List of all named arguments for this function. @@ -36,14 +40,14 @@ struct function_data_t { class function_info_t { public: - /// Function definition. - const wcstring definition; + /// Parsed source containing the function. + const parsed_source_ref_t parsed_source; + /// Node containing the function body, pointing into parsed_source. + const tnode_t body_node; /// Function description. Only the description 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; - /// Line where definition started. - const int definition_offset; /// List of all named arguments for this function. const wcstring_list_t named_arguments; /// Mapping of all variables that were inherited from the function definition scope to their @@ -55,18 +59,14 @@ class function_info_t { const bool shadow_scope; /// Constructs relevant information from the function_data. - function_info_t(const function_data_t &data, const wchar_t *filename, int def_offset, - bool autoload); + function_info_t(const function_data_t &data, const wchar_t *filename, bool autoload); /// Used by function_copy. - function_info_t(const function_info_t &data, const wchar_t *filename, int def_offset, - bool autoload); + function_info_t(const function_info_t &data, const wchar_t *filename, bool autoload); }; -/// Add a function. definition_line_offset is the line number of the function's definition within -/// its source file. -void function_add(const function_data_t &data, const parser_t &parser, - int definition_line_offset = 0); +/// Add a function. +void function_add(const function_data_t &data, const parser_t &parser); /// Remove the function with the specified name. void function_remove(const wcstring &name); @@ -112,7 +112,7 @@ const wchar_t *function_get_definition_file(const wcstring &name); /// /// This function does not autoload functions, it will only work on functions that have already been /// defined. -int function_get_definition_offset(const wcstring &name); +int function_get_definition_lineno(const wcstring &name); /// Returns a list of all named arguments of the specified function. wcstring_list_t function_get_named_arguments(const wcstring &name); diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 93cc240a2..a1fc4115c 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -327,7 +327,7 @@ parse_execution_result_t parse_execution_context_t::run_begin_statement( // Define a function. parse_execution_result_t parse_execution_context_t::run_function_statement( - tnode_t header, tnode_t block_end_command) { + tnode_t header, tnode_t body) { // Get arguments. wcstring_list_t arguments; argument_node_list_t arg_nodes = header.descendants(); @@ -337,30 +337,8 @@ parse_execution_result_t parse_execution_context_t::run_function_statement( if (result != parse_execution_success) { return result; } - - // The function definition extends from the end of the header to the function end. It's not - // just the range of the contents because that loses comments - see issue #1710. - assert(block_end_command.has_source()); - auto header_range = header.source_range(); - size_t contents_start = header_range->start + header_range->length; - size_t contents_end = block_end_command.source_range() - ->start; // 1 past the last character in the function definition - assert(contents_end >= contents_start); - - // Swallow whitespace at both ends. - while (contents_start < contents_end && iswspace(pstree->src.at(contents_start))) { - contents_start++; - } - while (contents_start < contents_end && iswspace(pstree->src.at(contents_end - 1))) { - contents_end--; - } - - assert(contents_end >= contents_start); - const wcstring contents_str = - wcstring(pstree->src, contents_start, contents_end - contents_start); - int definition_line_offset = this->line_offset_of_character_at_offset(contents_start); io_streams_t streams(0); // no limit on the amount of output from builtin_function() - int err = builtin_function(*parser, streams, arguments, contents_str, definition_line_offset); + int err = builtin_function(*parser, streams, arguments, pstree, body); proc_set_last_status(err); if (!streams.err.empty()) { @@ -382,8 +360,7 @@ parse_execution_result_t parse_execution_context_t::run_block_statement( } else if (auto header = bheader.try_get_child()) { ret = run_while_statement(header, contents); } else if (auto header = bheader.try_get_child()) { - tnode_t func_end = statement.child<2>(); - ret = run_function_statement(header, func_end); + ret = run_function_statement(header, contents); } else if (auto header = bheader.try_get_child()) { ret = run_begin_statement(contents); } else { diff --git a/src/parse_execution.h b/src/parse_execution.h index 24a0989e8..1b7f305d7 100644 --- a/src/parse_execution.h +++ b/src/parse_execution.h @@ -102,7 +102,7 @@ class parse_execution_context_t { parse_execution_result_t run_while_statement(tnode_t statement, tnode_t contents); parse_execution_result_t run_function_statement(tnode_t header, - tnode_t block_end); + tnode_t body); parse_execution_result_t run_begin_statement(tnode_t contents); enum globspec_t { failglob, nullglob }; diff --git a/src/parser.cpp b/src/parser.cpp index 4b91d5642..6f042f10a 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -489,7 +489,7 @@ int parser_t::get_lineno() const { // If we are executing a function, we have to add in its offset. const wchar_t *function_name = is_function(); if (function_name != NULL) { - lineno += function_get_definition_offset(function_name); + lineno += function_get_definition_lineno(function_name); } } return lineno; diff --git a/src/tnode.h b/src/tnode.h index c3658f606..0aa95dc15 100644 --- a/src/tnode.h +++ b/src/tnode.h @@ -97,12 +97,11 @@ class tnode_t { uint8_t child_count() const { return nodeptr ? nodeptr->child_count : 0; } maybe_t source_range() const { - if (!has_source()) return none(); + if (nodeptr->source_start == NODE_OFFSET_INVALID) return none(); return source_range_t{nodeptr->source_start, nodeptr->source_length}; } wcstring get_source(const wcstring &str) const { - assert(has_source() && "Source missing"); return nodeptr->get_source(str); } From 2fa705e4b26d16a511158811f338aa140b1d8076 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 10 Feb 2018 17:36:39 -0800 Subject: [PATCH 02/14] Use a for-in loop in function.cpp --- src/function.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/function.cpp b/src/function.cpp index eae881746..c35e6664d 100644 --- a/src/function.cpp +++ b/src/function.cpp @@ -157,9 +157,8 @@ void function_add(const function_data_t &data, const parser_t &parser) { loaded_functions.insert(new_pair); // Add event handlers. - for (std::vector::const_iterator iter = data.events.begin(); iter != data.events.end(); - ++iter) { - event_add_handler(*iter); + for (const event_t &event : data.events) { + event_add_handler(event); } } From 9cd24c042a1b6a7a9a4f40e1db9a4eb3322b533a Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 10 Feb 2018 17:38:57 -0800 Subject: [PATCH 03/14] Migrate function_info_t from header into .cpp file This struct was not used outside of the .cpp file. --- src/function.cpp | 28 ++++++++++++++++++++++++++++ src/function.h | 31 ++----------------------------- 2 files changed, 30 insertions(+), 29 deletions(-) diff --git a/src/function.cpp b/src/function.cpp index c35e6664d..bd0d4b107 100644 --- a/src/function.cpp +++ b/src/function.cpp @@ -29,6 +29,34 @@ #include "reader.h" #include "wutil.h" // IWYU pragma: keep +class function_info_t { +public: + /// Parsed source containing the function. + const parsed_source_ref_t parsed_source; + /// Node containing the function body, pointing into parsed_source. + const tnode_t body_node; + /// Function description. Only the description 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; + /// List of all named arguments for this function. + const wcstring_list_t named_arguments; + /// 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; + /// Set to true if invoking this function shadows the variables of the underlying function. + const bool shadow_scope; + + /// Constructs relevant information from the function_data. + function_info_t(const function_data_t &data, const wchar_t *filename, bool autoload); + + /// Used by function_copy. + function_info_t(const function_info_t &data, const wchar_t *filename, bool autoload); +}; + + /// Table containing all functions. typedef std::unordered_map function_map_t; static function_map_t loaded_functions; diff --git a/src/function.h b/src/function.h index 6d50107b9..637faaa45 100644 --- a/src/function.h +++ b/src/function.h @@ -16,8 +16,8 @@ class parser_t; /// 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_internal_data_t structure -/// is used for that purpose. Parhaps these two should be merged. +/// 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. struct function_data_t { /// Name of function. wcstring name; @@ -38,33 +38,6 @@ struct function_data_t { bool shadow_scope; }; -class function_info_t { - public: - /// Parsed source containing the function. - const parsed_source_ref_t parsed_source; - /// Node containing the function body, pointing into parsed_source. - const tnode_t body_node; - /// Function description. Only the description 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; - /// List of all named arguments for this function. - const wcstring_list_t named_arguments; - /// 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; - /// Set to true if invoking this function shadows the variables of the underlying function. - const bool shadow_scope; - - /// Constructs relevant information from the function_data. - function_info_t(const function_data_t &data, const wchar_t *filename, bool autoload); - - /// Used by function_copy. - function_info_t(const function_info_t &data, const wchar_t *filename, bool autoload); -}; - /// Add a function. void function_add(const function_data_t &data, const parser_t &parser); From 41ba0dfadbcaf92e9ae6f036e17d98fa6f8b7c14 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 10 Feb 2018 19:16:35 -0800 Subject: [PATCH 04/14] Evaluate tnode_t instead of parse_node_t This concerns block nodes with redirections, like begin ... end | grep ... Prior to this fix, we passed in a pointer to the node. Switch to passing in the tnode and parsed source ref. This improves type safety and better aligns with the function-node plans. --- src/exec.cpp | 16 +++--- src/parse_execution.cpp | 109 ++++++++++++++++++---------------------- src/parse_execution.h | 12 +++-- src/parser.cpp | 20 +++++--- src/parser.h | 6 ++- src/proc.cpp | 20 +------- src/proc.h | 30 +++++------ src/tnode.h | 3 ++ 8 files changed, 99 insertions(+), 117 deletions(-) diff --git a/src/exec.cpp b/src/exec.cpp index 91cf99cd6..8ceae0aab 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -306,13 +306,14 @@ static bool io_transmogrify(const io_chain_t &in_chain, io_chain_t *out_chain, /// clean up morphed redirections. /// /// \param def the code to evaluate, or the empty string if none -/// \param node_offset the offset of the node to evalute, or NODE_OFFSET_INVALID +/// \param statement the statement node to evaluate, or empty /// \param block_type the type of block to push on evaluation /// \param ios the io redirections to be performed on this block -static void internal_exec_helper(parser_t &parser, const wcstring &def, node_offset_t node_offset, +static void internal_exec_helper(parser_t &parser, const wcstring &def, + tnode_t statement, enum block_type_t block_type, const io_chain_t &ios) { - // If we have a valid node offset, then we must not have a string to execute. - assert(node_offset == NODE_OFFSET_INVALID || def.empty()); + // If we have a valid statement node, then we must not have a string to execute. + assert(!statement || def.empty()); io_chain_t morphed_chain; std::vector opened_fds; @@ -324,10 +325,10 @@ static void internal_exec_helper(parser_t &parser, const wcstring &def, node_off return; } - if (node_offset == NODE_OFFSET_INVALID) { + if (!statement) { parser.eval(def, morphed_chain, block_type); } else { - parser.eval_block_node(node_offset, morphed_chain, block_type); + parser.eval_node(statement, morphed_chain, block_type); } morphed_chain.clear(); @@ -810,8 +811,7 @@ void exec_job(parser_t &parser, job_t *j) { verify_buffer_output(); if (!exec_error) { - internal_exec_helper(parser, def, NODE_OFFSET_INVALID, TOP, - process_net_io_chain); + internal_exec_helper(parser, def, {}, TOP, process_net_io_chain); } parser.allow_function(); diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index a1fc4115c..5a0c10603 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -975,25 +975,27 @@ parse_execution_result_t parse_execution_context_t::populate_boolean_process( } template -parse_execution_result_t parse_execution_context_t::populate_block_process(job_t *job, - process_t *proc, - tnode_t node) { +parse_execution_result_t parse_execution_context_t::populate_block_process( + job_t *job, process_t *proc, tnode_t statement, + tnode_t specific_statement) { // We handle block statements by creating INTERNAL_BLOCK_NODE, that will bounce back to us when // it's time to execute them. UNUSED(job); static_assert(Type::token == symbol_block_statement || Type::token == symbol_if_statement || Type::token == symbol_switch_statement, "Invalid block process"); + assert(statement && "statement missing"); + assert(specific_statement && "specific_statement missing"); // The set of IO redirections that we construct for the process. // TODO: fix this ugly find_child. - auto arguments = node.template find_child(); + auto arguments = specific_statement.template find_child(); io_chain_t process_io_chain; bool errored = !this->determine_io_chain(arguments, &process_io_chain); if (errored) return parse_execution_errored; proc->type = INTERNAL_BLOCK_NODE; - proc->internal_block_node = this->get_offset(node); + proc->internal_block_node = statement; proc->set_io_chain(process_io_chain); return parse_execution_success; } @@ -1012,15 +1014,15 @@ parse_execution_result_t parse_execution_context_t::populate_job_process( } case symbol_block_statement: result = this->populate_block_process( - job, proc, tnode_t(&tree(), &specific_statement)); + job, proc, statement, tnode_t(&tree(), &specific_statement)); break; case symbol_if_statement: result = this->populate_block_process( - job, proc, tnode_t(&tree(), &specific_statement)); + job, proc, statement, tnode_t(&tree(), &specific_statement)); break; case symbol_switch_statement: result = this->populate_block_process( - job, proc, tnode_t(&tree(), &specific_statement)); + job, proc, statement, tnode_t(&tree(), &specific_statement)); break; case symbol_decorated_statement: { // Get the plain statement. It will pull out the decoration itself. @@ -1127,7 +1129,7 @@ parse_execution_result_t parse_execution_context_t::run_1_job(tnode_t jo } // When we encounter a block construct (e.g. while loop) in the general case, we create a "block - // process" that has a pointer to its source. This allows us to handle block-level redirections. + // process" containing its node. This allows us to handle block-level redirections. // However, if there are no redirections, then we can just jump into the block directly, which // is significantly faster. if (job_is_simple_block(job_node)) { @@ -1257,63 +1259,48 @@ parse_execution_result_t parse_execution_context_t::run_job_list(tnode_t j return result; } -parse_execution_result_t parse_execution_context_t::eval_node_at_offset( - node_offset_t offset, const block_t *associated_block, const io_chain_t &io) { - // Don't ever expect to have an empty tree if this is called. - assert(!tree().empty()); //!OCLINT(multiple unary operator) - assert(offset < tree().size()); - +parse_execution_result_t parse_execution_context_t::eval_node(tnode_t statement, + const block_t *associated_block, + const io_chain_t &io) { + assert(statement && "Empty node in eval_node"); + assert(statement.matches_node_tree(tree()) && "statement has unexpected tree"); // Apply this block IO for the duration of this function. scoped_push block_io_push(&block_io, io); - - const parse_node_t &node = tree().at(offset); - - // Currently, we only expect to execute the top level job list, or a block node. Assert that. - assert(node.type == symbol_job_list || specific_statement_type_is_redirectable_block(node)); - enum parse_execution_result_t status = parse_execution_success; - switch (node.type) { - case symbol_job_list: { - // We should only get a job list if it's the very first node. This is because this is - // the entry point for both top-level execution (the first node) and INTERNAL_BLOCK_NODE - // execution (which does block statements, but never job lists). - assert(offset == 0); - tnode_t job_list{&tree(), &node}; - wcstring func_name; - auto infinite_recursive_node = - this->infinite_recursive_statement_in_job_list(job_list, &func_name); - if (infinite_recursive_node) { - // We have an infinite recursion. - this->report_error(infinite_recursive_node, INFINITE_FUNC_RECURSION_ERR_MSG, - func_name.c_str()); - status = parse_execution_errored; - } else { - // No infinite recursion. - status = this->run_job_list(job_list, associated_block); - } - break; - } - case symbol_block_statement: { - status = this->run_block_statement({&tree(), &node}); - break; - } - case symbol_if_statement: { - status = this->run_if_statement({&tree(), &node}); - break; - } - case symbol_switch_statement: { - status = this->run_switch_statement({&tree(), &node}); - break; - } - default: { - // In principle, we could support other node types. However we never expect to be passed - // them - see above. - debug(0, "Unexpected node %ls found in %s", node.describe().c_str(), __FUNCTION__); - PARSER_DIE(); - break; - } + if (auto block = statement.try_get_child()) { + status = this->run_block_statement(block); + } else if (auto ifstat = statement.try_get_child()) { + status = this->run_if_statement(ifstat); + } else if (auto switchstat = statement.try_get_child()) { + status = this->run_switch_statement(switchstat); + } else { + debug(0, "Unexpected node %ls found in %s", statement.node()->describe().c_str(), + __FUNCTION__); + abort(); } + return status; +} +parse_execution_result_t parse_execution_context_t::eval_node(tnode_t job_list, + const block_t *associated_block, + const io_chain_t &io) { + // Apply this block IO for the duration of this function. + assert(job_list && "Empty node in eval_node"); + assert(job_list.matches_node_tree(tree()) && "job_list has unexpected tree"); + scoped_push block_io_push(&block_io, io); + enum parse_execution_result_t status = parse_execution_success; + wcstring func_name; + auto infinite_recursive_node = + this->infinite_recursive_statement_in_job_list(job_list, &func_name); + if (infinite_recursive_node) { + // We have an infinite recursion. + this->report_error(infinite_recursive_node, INFINITE_FUNC_RECURSION_ERR_MSG, + func_name.c_str()); + status = parse_execution_errored; + } else { + // No infinite recursion. + status = this->run_job_list(job_list, associated_block); + } return status; } diff --git a/src/parse_execution.h b/src/parse_execution.h index 1b7f305d7..17e178fcc 100644 --- a/src/parse_execution.h +++ b/src/parse_execution.h @@ -91,7 +91,8 @@ class parse_execution_context_t { template parse_execution_result_t populate_block_process(job_t *job, process_t *proc, - tnode_t statement_node); + tnode_t statement, + tnode_t specific_statement); // These encapsulate the actual logic of various (block) statements. parse_execution_result_t run_block_statement(tnode_t statement); @@ -146,11 +147,12 @@ class parse_execution_context_t { /// Return the parse tree. const parse_node_tree_t &tree() const { return pstree->tree; } - /// Start executing at the given node offset. Returns 0 if there was no error, 1 if there was an + /// Start executing at the given node. Returns 0 if there was no error, 1 if there was an /// error. - parse_execution_result_t eval_node_at_offset(node_offset_t offset, - const block_t *associated_block, - const io_chain_t &io); + parse_execution_result_t eval_node(tnode_t statement, + const block_t *associated_block, const io_chain_t &io); + parse_execution_result_t eval_node(tnode_t job_list, + const block_t *associated_block, const io_chain_t &io); }; #endif diff --git a/src/parser.cpp b/src/parser.cpp index 6f042f10a..9fdade353 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -670,7 +670,8 @@ int parser_t::eval(parsed_source_ref_t ps, const io_chain_t &io, enum block_type const parse_execution_context_t *ctx = execution_contexts.back().get(); // Execute the first node. - this->eval_block_node(0, io, block_type); + tnode_t start{&ps->tree, &ps->tree.front()}; + this->eval_node(start, io, block_type); // Clean up the execution context stack. assert(!execution_contexts.empty() && execution_contexts.back().get() == ctx); @@ -679,12 +680,11 @@ int parser_t::eval(parsed_source_ref_t ps, const io_chain_t &io, enum block_type return 0; } -int parser_t::eval_block_node(node_offset_t node_idx, const io_chain_t &io, - enum block_type_t block_type) { - // Paranoia. It's a little frightening that we're given only a node_idx and we interpret this in - // the topmost execution context's tree. What happens if two trees were to be interleaved? - // Fortunately that cannot happen (yet); in the future we probably want some sort of reference - // counted trees. +template +int parser_t::eval_node(tnode_t node, const io_chain_t &io, enum block_type_t block_type) { + static_assert( + std::is_same::value || std::is_same::value, + "Unexpected node type"); parse_execution_context_t *ctx = execution_contexts.back().get(); assert(ctx != NULL); @@ -711,13 +711,17 @@ int parser_t::eval_block_node(node_offset_t node_idx, const io_chain_t &io, // Start it up scope_block_t *scope_block = this->push_block(block_type); - int result = ctx->eval_node_at_offset(node_idx, scope_block, io); + int result = ctx->eval_node(node, scope_block, io); this->pop_block(scope_block); job_reap(0); // reap again return result; } +// Explicit instantiations. TODO: use overloads instead? +template int parser_t::eval_node(tnode_t, const io_chain_t &io, enum block_type_t block_type); +template int parser_t::eval_node(tnode_t, const io_chain_t &io, enum block_type_t block_type); + bool parser_t::detect_errors_in_argument_list(const wcstring &arg_list_src, wcstring *out, const wchar_t *prefix) { bool errored = false; diff --git a/src/parser.h b/src/parser.h index 5fcf6a849..04e36e278 100644 --- a/src/parser.h +++ b/src/parser.h @@ -251,8 +251,10 @@ class parser_t { /// Evaluate the parsed source ps. int eval(parsed_source_ref_t ps, const io_chain_t &io, enum block_type_t block_type); - /// Evaluates a block node at the given node offset in the topmost execution context. - int eval_block_node(node_offset_t node_idx, const io_chain_t &io, enum block_type_t block_type); + /// Evaluates a node in the topmost execution context. + /// The node type must be grammar::statement or grammar::job_list. + template + int eval_node(tnode_t node, const io_chain_t &io, enum block_type_t block_type); /// Evaluate line as a list of parameters, i.e. tokenize it and perform parameter expansion and /// cmdsubst execution on the tokens. The output is inserted into output. Errors are ignored. diff --git a/src/proc.cpp b/src/proc.cpp index a9b313656..1e77e4561 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -344,25 +344,7 @@ static void handle_child_status(pid_t pid, int status) { return; } -process_t::process_t() - : is_first_in_job(), - is_last_in_job(), - type(), // gets set later - internal_block_node(NODE_OFFSET_INVALID), - pid(0), - pipe_write_fd(0), - pipe_read_fd(0), - completed(0), - stopped(0), - status(0), - count_help_magic(0) -#ifdef HAVE__PROC_SELF_STAT - , - last_time(), - last_jiffies(0) -#endif -{ -} +process_t::process_t() {} /// The constructor sets the pgid to -2 as a sentinel value /// 0 should not be used; although it is not a valid PGID in userspace, diff --git a/src/proc.h b/src/proc.h index 52c956571..227b45599 100644 --- a/src/proc.h +++ b/src/proc.h @@ -18,6 +18,7 @@ #include "common.h" #include "io.h" #include "parse_tree.h" +#include "tnode.h" /// Types of processes. enum process_type_t { @@ -73,15 +74,16 @@ class process_t { process_t(); // Note whether we are the first and/or last in the job - bool is_first_in_job; - bool is_last_in_job; + bool is_first_in_job{false}; + bool is_last_in_job{false}; /// Type of process. Can be one of \c EXTERNAL, \c INTERNAL_BUILTIN, \c INTERNAL_FUNCTION, \c /// INTERNAL_EXEC. - enum process_type_t type; + enum process_type_t type { EXTERNAL }; - /// For internal block processes only, the node offset of the block. - node_offset_t internal_block_node; + /// For internal block processes only, the node offset of the statement. + /// This is always either block, ifs, or switchs, never boolean or decorated. + tnode_t internal_block_node{}; /// Sets argv. void set_argv(const wcstring_list_t &argv) { argv_array.set(argv); } @@ -111,24 +113,24 @@ class process_t { /// Actual command to pass to exec in case of EXTERNAL or INTERNAL_EXEC. wcstring actual_cmd; /// Process ID - pid_t pid; + pid_t pid{0}; /// File descriptor that pipe output should bind to. - int pipe_write_fd; + int pipe_write_fd{0}; /// File descriptor that the _next_ process pipe input should bind to. - int pipe_read_fd; + int pipe_read_fd{0}; /// True if process has completed. - volatile int completed; + volatile int completed{false}; /// True if process has stopped. - volatile int stopped; + volatile int stopped{false}; /// Reported status value. - volatile int status; + volatile int status{0}; /// Special flag to tell the evaluation function for count to print the help information. - int count_help_magic; + int count_help_magic{0}; #ifdef HAVE__PROC_SELF_STAT /// Last time of cpu time check. - struct timeval last_time; + struct timeval last_time {}; /// Number of jiffies spent in process at last cpu time check. - unsigned long last_jiffies; + unsigned long last_jiffies{0}; #endif }; diff --git a/src/tnode.h b/src/tnode.h index 0aa95dc15..95c8f129f 100644 --- a/src/tnode.h +++ b/src/tnode.h @@ -88,6 +88,9 @@ class tnode_t { bool operator!=(const tnode_t &rhs) const { return !(*this == rhs); } + // Helper to return whether the given tree is the same as ours. + bool matches_node_tree(const parse_node_tree_t &t) const { return &t == tree; } + bool has_source() const { return nodeptr && nodeptr->has_source(); } // return the tag, or 0 if missing. From db33ed0fc757bfcf7b2fffc0b4dda049d5a397c3 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 11 Feb 2018 16:37:38 -0800 Subject: [PATCH 05/14] Migrate some function properties into a shared_ptr The idea is that we can return the shared pointer directly, avoiding lots of annoying little getter functions that each need to take locks. It also helps to pull together the data structures used to initialize functions versus store them. --- src/builtin_function.cpp | 17 ++++------ src/function.cpp | 71 ++++++++++++++++------------------------ src/function.h | 29 ++++++++++------ 3 files changed, 55 insertions(+), 62 deletions(-) diff --git a/src/builtin_function.cpp b/src/builtin_function.cpp index e865c2a4c..ca66b301c 100644 --- a/src/builtin_function.cpp +++ b/src/builtin_function.cpp @@ -249,23 +249,20 @@ int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_lis if (!opts.description.empty()) d.description = opts.description; // d.description = opts.description; d.events.swap(opts.events); - d.shadow_scope = opts.shadow_scope; - d.named_arguments.swap(opts.named_arguments); - d.inherit_vars.swap(opts.inherit_vars); + d.props.shadow_scope = opts.shadow_scope; + d.props.named_arguments = std::move(opts.named_arguments); + d.inherit_vars = std::move(opts.inherit_vars); for (size_t i = 0; i < d.events.size(); i++) { event_t &e = d.events.at(i); e.function_name = d.name; } - d.parsed_source = source; - d.body_node = body; - function_add(d, parser); + d.props.parsed_source = source; + d.props.body_node = body; + function_add(std::move(d), parser); // Handle wrap targets by creating the appropriate completions. - for (size_t w = 0; w < opts.wrap_targets.size(); w++) { - complete_add_wrapper(function_name, opts.wrap_targets.at(w)); - } - + for (const wcstring &wt : opts.wrap_targets) complete_add_wrapper(function_name, wt); return STATUS_CMD_OK; } diff --git a/src/function.cpp b/src/function.cpp index bd0d4b107..a39a34871 100644 --- a/src/function.cpp +++ b/src/function.cpp @@ -31,29 +31,23 @@ class function_info_t { public: - /// Parsed source containing the function. - const parsed_source_ref_t parsed_source; - /// Node containing the function body, pointing into parsed_source. - const tnode_t body_node; - /// Function description. Only the description 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; - /// List of all named arguments for this function. - const wcstring_list_t named_arguments; - /// 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; - /// Set to true if invoking this function shadows the variables of the underlying function. - const bool shadow_scope; + /// Immutable properties of the function. + std::shared_ptr 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(const function_data_t &data, const wchar_t *filename, bool autoload); + /// Constructs relevant information from the function_data. + function_info_t(function_data_t data, const wchar_t *filename, bool autoload); - /// Used by function_copy. - function_info_t(const function_info_t &data, const wchar_t *filename, bool autoload); + /// Used by function_copy. + function_info_t(const function_info_t &data, const wchar_t *filename, bool autoload); }; @@ -138,34 +132,27 @@ static void autoload_names(std::unordered_set &names, int get_hidden) static std::map snapshot_vars(const wcstring_list_t &vars) { std::map result; - for (wcstring_list_t::const_iterator it = vars.begin(), end = vars.end(); it != end; ++it) { - auto var = env_get(*it); - if (var) result.insert(std::make_pair(*it, std::move(*var))); + for (const wcstring &name : vars) { + auto var = env_get(name); + if (var) result[name] = std::move(*var); } return result; } -function_info_t::function_info_t(const function_data_t &data, const wchar_t *filename, - bool autoload) - : parsed_source(data.parsed_source), - body_node(data.body_node), - description(data.description), +function_info_t::function_info_t(function_data_t data, const wchar_t *filename, bool autoload) + : props(std::make_shared(std::move(data.props))), + description(std::move(data.description)), definition_file(intern(filename)), - named_arguments(data.named_arguments), inherit_vars(snapshot_vars(data.inherit_vars)), - is_autoload(autoload), - shadow_scope(data.shadow_scope) {} + is_autoload(autoload) {} function_info_t::function_info_t(const function_info_t &data, const wchar_t *filename, bool autoload) - : parsed_source(data.parsed_source), - body_node(data.body_node), + : props(data.props), description(data.description), definition_file(intern(filename)), - named_arguments(data.named_arguments), inherit_vars(data.inherit_vars), - is_autoload(autoload), - shadow_scope(data.shadow_scope) {} + is_autoload(autoload) {} void function_add(const function_data_t &data, const parser_t &parser) { UNUSED(parser); @@ -250,7 +237,7 @@ bool function_get_definition(const wcstring &name, wcstring *out_definition) { scoped_rlock locker(functions_lock); const function_info_t *func = function_get(name); if (func && out_definition) { - out_definition->assign(func->body_node.get_source(func->parsed_source->src)); + out_definition->assign(func->props->body_node.get_source(func->props->parsed_source->src)); } return func != NULL; } @@ -258,7 +245,7 @@ bool function_get_definition(const wcstring &name, wcstring *out_definition) { wcstring_list_t function_get_named_arguments(const wcstring &name) { scoped_rlock locker(functions_lock); const function_info_t *func = function_get(name); - return func ? func->named_arguments : wcstring_list_t(); + return func ? func->props->named_arguments : wcstring_list_t(); } std::map function_get_inherit_vars(const wcstring &name) { @@ -270,7 +257,7 @@ std::map function_get_inherit_vars(const wcstring &name) { bool function_get_shadow_scope(const wcstring &name) { scoped_rlock locker(functions_lock); const function_info_t *func = function_get(name); - return func ? func->shadow_scope : false; + return func ? func->props->shadow_scope : false; } bool function_get_desc(const wcstring &name, wcstring *out_desc) { @@ -344,12 +331,12 @@ int function_get_definition_lineno(const wcstring &name) { if (!func) return -1; // return one plus the number of newlines at offsets less than the start of our function's statement (which includes the header). // TODO: merge with line_offset_of_character_at_offset? - auto block_stat = func->body_node.try_get_parent(); + auto block_stat = func->props->body_node.try_get_parent(); assert(block_stat && "Function body is not part of block statement"); auto source_range = block_stat.source_range(); assert(source_range && "Function has no source range"); uint32_t func_start = source_range->start; - const wcstring &source = func->parsed_source->src; + const wcstring &source = func->props->parsed_source->src; assert(func_start <= source.size() && "function start out of bounds"); return 1 + std::count(source.begin(), source.begin() + func_start, L'\n'); } diff --git a/src/function.h b/src/function.h index 637faaa45..ed796df50 100644 --- a/src/function.h +++ b/src/function.h @@ -15,6 +15,21 @@ class parser_t; +/// A function's constant properties. These do not change once initialized. +struct function_properties_t { + /// Parsed source containing the function. + parsed_source_ref_t parsed_source; + + /// Node containing the function body, pointing into parsed_source. + tnode_t body_node; + + /// List of all named arguments for this function. + wcstring_list_t named_arguments; + + /// Set to true if invoking this function shadows the variables of the underlying function. + bool shadow_scope; +}; + /// 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. @@ -23,19 +38,13 @@ struct function_data_t { wcstring name; /// Description of function. wcstring description; - /// Parsed source containing the function. - parsed_source_ref_t parsed_source; - /// Node containing the function body, pointing into parsed_source. - tnode_t body_node; - /// List of all event handlers for this function. - std::vector events; - /// List of all named arguments for this function. - wcstring_list_t named_arguments; /// 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; - /// Set to true if invoking this function shadows the variables of the underlying function. - bool shadow_scope; + /// Function's metadata fields + function_properties_t props; + /// List of all event handlers for this function. + std::vector events; }; /// Add a function. From 976514597d4e8dfe07a49a83abdac2f80ba04621 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 11 Feb 2018 17:21:24 -0800 Subject: [PATCH 06/14] Migrate function getters to use function_get_properties This replaces some of the teensy function getters with the function that just returns a shared_ptr to the properties struct. --- src/builtin_functions.cpp | 30 ++++++++++++++---------------- src/exec.cpp | 16 ++++++++-------- src/function.cpp | 37 +++++++++++++++++-------------------- src/function.h | 15 +++++++-------- 4 files changed, 46 insertions(+), 52 deletions(-) diff --git a/src/builtin_functions.cpp b/src/builtin_functions.cpp index c1142a646..c8433d0b2 100644 --- a/src/builtin_functions.cpp +++ b/src/builtin_functions.cpp @@ -134,12 +134,13 @@ static wcstring functions_def(const wcstring &name) { out.append(esc_desc); } - if (!function_get_shadow_scope(name)) { + auto props = function_get_properties(name); + assert(props && "Should have function properties"); + if (!props->shadow_scope) { out.append(L" --no-scope-shadowing"); } - for (size_t i = 0; i < ev.size(); i++) { - const event_t *next = ev.at(i).get(); + for (const auto &next : ev) { switch (next->type) { case EVENT_SIGNAL: { append_format(out, L" --on-signal %ls", sig2wcs(next->param1.signal)); @@ -172,11 +173,11 @@ static wcstring functions_def(const wcstring &name) { } } - wcstring_list_t named = function_get_named_arguments(name); + const wcstring_list_t &named = props->named_arguments; if (!named.empty()) { append_format(out, L" --argument"); - for (size_t i = 0; i < named.size(); i++) { - append_format(out, L" %ls", named.at(i).c_str()); + for (const auto &name : named) { + append_format(out, L" %ls", name.c_str()); } } @@ -188,17 +189,14 @@ 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 (std::map::const_iterator it = inherit_vars.begin(), - end = inherit_vars.end(); - it != end; ++it) { + for (const auto &kv : inherit_vars) { wcstring_list_t lst; - it->second.to_list(lst); + kv.second.to_list(lst); // This forced tab is crummy, but we don't know what indentation style the function uses. - append_format(out, L"\n\tset -l %ls", it->first.c_str()); - for (wcstring_list_t::const_iterator arg_it = lst.begin(), arg_end = lst.end(); - arg_it != arg_end; ++arg_it) { - wcstring earg = escape_string(*arg_it, ESCAPE_ALL); + append_format(out, L"\n\tset -l %ls", kv.first.c_str()); + for (const auto &arg : lst) { + wcstring earg = escape_string(arg, ESCAPE_ALL); out.push_back(L' '); out.append(earg); } @@ -224,6 +222,7 @@ static int report_function_metadata(const wchar_t *funcname, bool verbose, io_st int line_number = 0; if (function_exists(funcname)) { + auto props = function_get_properties(funcname); path = function_get_definition_file(funcname); if (path) { autoloaded = function_is_autoloaded(funcname) ? L"autoloaded" : L"not-autoloaded"; @@ -231,8 +230,7 @@ static int report_function_metadata(const wchar_t *funcname, bool verbose, io_st } else { path = L"stdin"; } - shadows_scope = - function_get_shadow_scope(funcname) ? L"scope-shadowing" : L"no-scope-shadowing"; + shadows_scope = props->shadow_scope ? L"scope-shadowing" : L"no-scope-shadowing"; function_get_desc(funcname, &description); description = escape_string(description, ESCAPE_NO_QUOTED); } diff --git a/src/exec.cpp b/src/exec.cpp index 8ceae0aab..e329a9646 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -792,19 +792,19 @@ void exec_job(parser_t &parser, job_t *j) { switch (p->type) { case INTERNAL_FUNCTION: { const wcstring func_name = p->argv0(); - wcstring def; - bool function_exists = function_get_definition(func_name, &def); - bool shadow_scope = function_get_shadow_scope(func_name); - const std::map inherit_vars = - function_get_inherit_vars(func_name); - - if (!function_exists) { + auto props = function_get_properties(func_name); + if (!props) { debug(0, _(L"Unknown function '%ls'"), p->argv0()); break; } + wcstring def; + function_get_definition(func_name, &def); + const std::map inherit_vars = + function_get_inherit_vars(func_name); + function_block_t *fb = - parser.push_block(p, func_name, shadow_scope); + parser.push_block(p, func_name, props->shadow_scope); function_prepare_environment(func_name, p->get_argv() + 1, inherit_vars); parser.forbid_function(func_name); diff --git a/src/function.cpp b/src/function.cpp index a39a34871..bd38b083e 100644 --- a/src/function.cpp +++ b/src/function.cpp @@ -177,6 +177,16 @@ void function_add(const function_data_t &data, const parser_t &parser) { } } +std::shared_ptr function_get_properties(const wcstring &name) { + if (parser_keywords_is_reserved(name)) return nullptr; + scoped_rlock locker(functions_lock); + auto where = loaded_functions.find(name); + if (where != loaded_functions.end()) { + return where->second.props; + } + return nullptr; +} + int function_exists(const wcstring &cmd) { if (parser_keywords_is_reserved(cmd)) return 0; scoped_rlock locker(functions_lock); @@ -242,24 +252,12 @@ bool function_get_definition(const wcstring &name, wcstring *out_definition) { return func != NULL; } -wcstring_list_t function_get_named_arguments(const wcstring &name) { - scoped_rlock locker(functions_lock); - const function_info_t *func = function_get(name); - return func ? func->props->named_arguments : wcstring_list_t(); -} - std::map function_get_inherit_vars(const wcstring &name) { scoped_rlock locker(functions_lock); const function_info_t *func = function_get(name); return func ? func->inherit_vars : std::map(); } -bool function_get_shadow_scope(const wcstring &name) { - scoped_rlock locker(functions_lock); - const function_info_t *func = function_get(name); - return func ? func->props->shadow_scope : false; -} - bool function_get_desc(const wcstring &name, wcstring *out_desc) { // Empty length string goes to NULL. scoped_rlock locker(functions_lock); @@ -348,21 +346,20 @@ int function_get_definition_lineno(const wcstring &name) { void function_prepare_environment(const wcstring &name, const wchar_t *const *argv, const std::map &inherited_vars) { env_set_argv(argv); - - const wcstring_list_t named_arguments = function_get_named_arguments(name); - if (!named_arguments.empty()) { + auto props = function_get_properties(name); + if (props && !props->named_arguments.empty()) { const wchar_t *const *arg = argv; - for (size_t i = 0; i < named_arguments.size(); i++) { + for (const wcstring &named_arg : props->named_arguments) { if (*arg) { - env_set_one(named_arguments.at(i), ENV_LOCAL | ENV_USER, *arg); + env_set_one(named_arg, ENV_LOCAL | ENV_USER, *arg); arg++; } else { - env_set_empty(named_arguments.at(i), ENV_LOCAL | ENV_USER); + env_set_empty(named_arg, ENV_LOCAL | ENV_USER); } } } - for (auto it = inherited_vars.begin(), end = inherited_vars.end(); it != end; ++it) { - env_set(it->first, ENV_LOCAL | ENV_USER, it->second.as_list()); + for (const auto &kv : inherited_vars) { + env_set(kv.first, ENV_LOCAL | ENV_USER, kv.second.as_list()); } } diff --git a/src/function.h b/src/function.h index ed796df50..078671b51 100644 --- a/src/function.h +++ b/src/function.h @@ -53,18 +53,24 @@ void function_add(const function_data_t &data, const parser_t &parser); /// Remove the function with the specified name. 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); + /// 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. +/// This does not trigger autoloading. bool function_get_definition(const wcstring &name, wcstring *out_definition); /// Returns by reference the description of the function with the name \c name. Returns true if the /// function exists and has a nonempty description, false if it does not. +/// This does not trigger autoloading. bool function_get_desc(const wcstring &name, wcstring *out_desc); /// Sets the description of the function with the name \c name. void function_set_desc(const wcstring &name, const wcstring &desc); /// Returns true if the function with the name name exists. +/// This may autoload. int function_exists(const wcstring &name); /// Attempts to load a function if not yet loaded. This is used by the completion machinery. @@ -91,14 +97,9 @@ bool function_is_autoloaded(const wcstring &name); const wchar_t *function_get_definition_file(const wcstring &name); /// Returns the linenumber where the definition of the specified function started. -/// -/// This function does not autoload functions, it will only work on functions that have already been -/// defined. +/// This does not trigger autoloading. int function_get_definition_lineno(const wcstring &name); -/// Returns a list of all named arguments of the specified function. -wcstring_list_t function_get_named_arguments(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); @@ -107,8 +108,6 @@ std::map function_get_inherit_vars(const wcstring &name); /// is successful. bool function_copy(const wcstring &name, const wcstring &new_name); -/// Returns whether this function shadows variables of the underlying function. -bool function_get_shadow_scope(const wcstring &name); /// Prepares the environment for executing a function. void function_prepare_environment(const wcstring &name, const wchar_t *const *argv, From c3f1961e36cd3be2327bd1c56a856c0b2077d52c Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 11 Feb 2018 19:34:12 -0800 Subject: [PATCH 07/14] Stop copying out function definition when executing a function This switches function execution from the function's source code to its stored node and pstree. This means we no longer have to re-parse the function every time we execute it. --- src/exec.cpp | 29 +++++++++++--------------- src/parse_execution.cpp | 1 + src/parser.cpp | 45 +++++++++++++++++++++-------------------- src/parser.h | 5 +++-- src/proc.h | 1 + tests/test_cmdsub.err | 4 ++-- 6 files changed, 42 insertions(+), 43 deletions(-) diff --git a/src/exec.cpp b/src/exec.cpp index e329a9646..b201ab5ba 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -305,15 +305,13 @@ static bool io_transmogrify(const io_chain_t &in_chain, io_chain_t *out_chain, /// Morph an io redirection chain into redirections suitable for passing to eval, call eval, and /// clean up morphed redirections. /// -/// \param def the code to evaluate, or the empty string if none -/// \param statement the statement node to evaluate, or empty -/// \param block_type the type of block to push on evaluation +/// \param parsed_source the parsed source code containing the node to evaluate +/// \param node the node to evaluate /// \param ios the io redirections to be performed on this block -static void internal_exec_helper(parser_t &parser, const wcstring &def, - tnode_t statement, - enum block_type_t block_type, const io_chain_t &ios) { - // If we have a valid statement node, then we must not have a string to execute. - assert(!statement || def.empty()); +template +void internal_exec_helper(parser_t &parser, parsed_source_ref_t parsed_source, tnode_t node, + const io_chain_t &ios) { + assert(parsed_source && node && "exec_helper missing source or without node"); io_chain_t morphed_chain; std::vector opened_fds; @@ -325,11 +323,7 @@ static void internal_exec_helper(parser_t &parser, const wcstring &def, return; } - if (!statement) { - parser.eval(def, morphed_chain, block_type); - } else { - parser.eval_node(statement, morphed_chain, block_type); - } + parser.eval_node(parsed_source, node, morphed_chain, TOP); morphed_chain.clear(); io_cleanup_fds(opened_fds); @@ -798,8 +792,6 @@ void exec_job(parser_t &parser, job_t *j) { break; } - wcstring def; - function_get_definition(func_name, &def); const std::map inherit_vars = function_get_inherit_vars(func_name); @@ -811,7 +803,8 @@ void exec_job(parser_t &parser, job_t *j) { verify_buffer_output(); if (!exec_error) { - internal_exec_helper(parser, def, {}, TOP, process_net_io_chain); + internal_exec_helper(parser, props->parsed_source, props->body_node, + process_net_io_chain); } parser.allow_function(); @@ -824,7 +817,9 @@ void exec_job(parser_t &parser, job_t *j) { verify_buffer_output(); if (!exec_error) { - internal_exec_helper(parser, wcstring(), p->internal_block_node, TOP, + assert(p->block_node_source && p->internal_block_node && + "Process is missing node info"); + internal_exec_helper(parser, p->block_node_source, p->internal_block_node, process_net_io_chain); } break; diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 5a0c10603..abcf2f9a1 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -995,6 +995,7 @@ parse_execution_result_t parse_execution_context_t::populate_block_process( if (errored) return parse_execution_errored; proc->type = INTERNAL_BLOCK_NODE; + proc->block_node_source = pstree; proc->internal_block_node = statement; proc->set_io_chain(process_io_chain); return parse_execution_success; diff --git a/src/parser.cpp b/src/parser.cpp index 9fdade353..9017a120c 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -658,36 +658,19 @@ int parser_t::eval(parsed_source_ref_t ps, const io_chain_t &io, enum block_type return 0; } - // Determine the initial eval level. If this is the first context, it's -1; otherwise it's the - // eval level of the top context. This is sort of wonky because we're stitching together a - // global notion of eval level from these separate objects. A better approach would be some - // profile object that all contexts share, and that tracks the eval levels on its own. - int exec_eval_level = - (execution_contexts.empty() ? -1 : execution_contexts.back()->current_eval_level()); - - // Append to the execution context stack. - execution_contexts.push_back(make_unique(ps, this, exec_eval_level)); - const parse_execution_context_t *ctx = execution_contexts.back().get(); - // Execute the first node. tnode_t start{&ps->tree, &ps->tree.front()}; - this->eval_node(start, io, block_type); - - // Clean up the execution context stack. - assert(!execution_contexts.empty() && execution_contexts.back().get() == ctx); - execution_contexts.pop_back(); + this->eval_node(ps, start, io, block_type); return 0; } template -int parser_t::eval_node(tnode_t node, const io_chain_t &io, enum block_type_t block_type) { +int parser_t::eval_node(parsed_source_ref_t ps, tnode_t node, const io_chain_t &io, + enum block_type_t block_type) { static_assert( std::is_same::value || std::is_same::value, "Unexpected node type"); - parse_execution_context_t *ctx = execution_contexts.back().get(); - assert(ctx != NULL); - CHECK_BLOCK(1); // Handle cancellation requests. If our block stack is currently empty, then we already did @@ -711,16 +694,34 @@ int parser_t::eval_node(tnode_t node, const io_chain_t &io, enum block_type_t // Start it up scope_block_t *scope_block = this->push_block(block_type); + + // Determine the initial eval level. If this is the first context, it's -1; otherwise it's the + // eval level of the top context. This is sort of wonky because we're stitching together a + // global notion of eval level from these separate objects. A better approach would be some + // profile object that all contexts share, and that tracks the eval levels on its own. + int exec_eval_level = + (execution_contexts.empty() ? -1 : execution_contexts.back()->current_eval_level()); + + // Append to the execution context stack. + execution_contexts.push_back(make_unique(ps, this, exec_eval_level)); + parse_execution_context_t *ctx = execution_contexts.back().get(); + int result = ctx->eval_node(node, scope_block, io); this->pop_block(scope_block); + // Clean up the execution context stack. + assert(!execution_contexts.empty() && execution_contexts.back().get() == ctx); + execution_contexts.pop_back(); + job_reap(0); // reap again return result; } // Explicit instantiations. TODO: use overloads instead? -template int parser_t::eval_node(tnode_t, const io_chain_t &io, enum block_type_t block_type); -template int parser_t::eval_node(tnode_t, const io_chain_t &io, enum block_type_t block_type); +template int parser_t::eval_node(parsed_source_ref_t, tnode_t, + const io_chain_t &, enum block_type_t); +template int parser_t::eval_node(parsed_source_ref_t, tnode_t, + const io_chain_t &, enum block_type_t); bool parser_t::detect_errors_in_argument_list(const wcstring &arg_list_src, wcstring *out, const wchar_t *prefix) { diff --git a/src/parser.h b/src/parser.h index 04e36e278..edb51dcf6 100644 --- a/src/parser.h +++ b/src/parser.h @@ -251,10 +251,11 @@ class parser_t { /// Evaluate the parsed source ps. int eval(parsed_source_ref_t ps, const io_chain_t &io, enum block_type_t block_type); - /// Evaluates a node in the topmost execution context. + /// Evaluates a node. /// The node type must be grammar::statement or grammar::job_list. template - int eval_node(tnode_t node, const io_chain_t &io, enum block_type_t block_type); + int eval_node(parsed_source_ref_t ps, tnode_t node, const io_chain_t &io, + enum block_type_t block_type); /// Evaluate line as a list of parameters, i.e. tokenize it and perform parameter expansion and /// cmdsubst execution on the tokens. The output is inserted into output. Errors are ignored. diff --git a/src/proc.h b/src/proc.h index 227b45599..74f8ebe41 100644 --- a/src/proc.h +++ b/src/proc.h @@ -83,6 +83,7 @@ class process_t { /// For internal block processes only, the node offset of the statement. /// This is always either block, ifs, or switchs, never boolean or decorated. + parsed_source_ref_t block_node_source{}; tnode_t internal_block_node{}; /// Sets argv. diff --git a/tests/test_cmdsub.err b/tests/test_cmdsub.err index cc56e58f2..1d39d0ae7 100644 --- a/tests/test_cmdsub.err +++ b/tests/test_cmdsub.err @@ -13,8 +13,8 @@ set b (string repeat -n 512 x) # Command sub over the limit should fail fish: Too much data emitted by command substitution so it was discarded -set -l x (string repeat -n $argv x) - ^ + set -l x (string repeat -n $argv x) + ^ in function 'subme' called on standard input with parameter list '513' From 8251d949f166fcf5295c80487b78bd1949c4c0f7 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 11 Feb 2018 20:08:40 -0800 Subject: [PATCH 08/14] Switch executing_node_idx to storing a tnode_t Avoids annoying index<->node conversions. --- src/parse_execution.cpp | 22 ++++++++++------------ src/parse_execution.h | 9 ++++----- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index abcf2f9a1..050d01891 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -1120,7 +1120,7 @@ parse_execution_result_t parse_execution_context_t::run_1_job(tnode_t jo scoped_push saved_eval_level(&eval_level, eval_level + 1); // Save the node index. - scoped_push saved_node_offset(&executing_node_idx, this->get_offset(job_node)); + scoped_push> saved_node(&executing_job_node, job_node); // Profiling support. long long start_time = 0, parse_time = 0, exec_time = 0; @@ -1305,20 +1305,19 @@ parse_execution_result_t parse_execution_context_t::eval_node(tnode_t node) { // If we're not executing anything, return -1. - if (requested_index == NODE_OFFSET_INVALID) { + if (!node) { return -1; } // If for some reason we're executing a node without source, return -1. - const parse_node_t &node = tree().at(requested_index); - if (!node.has_source()) { + auto range = node.source_range(); + if (!range) { return -1; } - size_t char_offset = tree().at(requested_index).source_start; - return this->line_offset_of_character_at_offset(char_offset); + return this->line_offset_of_character_at_offset(range->start); } int parse_execution_context_t::line_offset_of_character_at_offset(size_t offset) { @@ -1357,7 +1356,7 @@ int parse_execution_context_t::line_offset_of_character_at_offset(size_t offset) int parse_execution_context_t::get_current_line_number() { int line_number = -1; - int line_offset = this->line_offset_of_node_at_offset(this->executing_node_idx); + int line_offset = this->line_offset_of_node(this->executing_job_node); if (line_offset >= 0) { // The offset is 0 based; the number is 1 based. line_number = line_offset + 1; @@ -1367,10 +1366,9 @@ int parse_execution_context_t::get_current_line_number() { int parse_execution_context_t::get_current_source_offset() const { int result = -1; - if (executing_node_idx != NODE_OFFSET_INVALID) { - const parse_node_t &node = tree().at(executing_node_idx); - if (node.has_source()) { - result = static_cast(node.source_start); + if (executing_job_node) { + if (auto range = executing_job_node.source_range()) { + result = static_cast(range->start); } } return result; diff --git a/src/parse_execution.h b/src/parse_execution.h index 17e178fcc..7b99691a9 100644 --- a/src/parse_execution.h +++ b/src/parse_execution.h @@ -33,8 +33,8 @@ class parse_execution_context_t { parser_t *const parser; // parse_error_list_t errors; int eval_level; - // The currently executing node index, used to indicate the line number. - node_offset_t executing_node_idx = NODE_OFFSET_INVALID; + // The currently executing job node, used to indicate the line number. + tnode_t executing_job_node{}; // Cached line number information. size_t cached_lineno_offset = 0; int cached_lineno_count = 0; @@ -123,9 +123,8 @@ class parse_execution_context_t { parse_execution_result_t populate_job_from_job_node(job_t *j, tnode_t job_node, const block_t *associated_block); - // Returns the line number of the node at the given index, indexed from 0. Not const since it - // touches cached_lineno_offset. - int line_offset_of_node_at_offset(node_offset_t idx); + // Returns the line number of the node. Not const since it touches cached_lineno_offset. + int line_offset_of_node(tnode_t node); int line_offset_of_character_at_offset(size_t char_idx); public: From 3e063e7c1388a4174e835fb6afc382a73b68557d Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 11 Feb 2018 20:10:57 -0800 Subject: [PATCH 09/14] Remove node_offset from block_t It was not used. Also clean up the constructor some. --- src/parse_execution.cpp | 2 -- src/parser.cpp | 12 +----------- src/parser.h | 21 ++++++++------------- 3 files changed, 9 insertions(+), 26 deletions(-) diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 050d01891..1c6741df5 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -241,7 +241,6 @@ parse_execution_result_t parse_execution_context_t::run_if_statement( tnode_t statement) { // Push an if block. if_block_t *ib = parser->push_block(); - ib->node_offset = this->get_offset(*statement); parse_execution_result_t result = parse_execution_success; @@ -536,7 +535,6 @@ parse_execution_result_t parse_execution_context_t::run_while_statement( tnode_t header, tnode_t contents) { // Push a while block. while_block_t *wb = parser->push_block(); - wb->node_offset = this->get_offset(header); parse_execution_result_t ret = parse_execution_success; diff --git a/src/parser.cpp b/src/parser.cpp index 9017a120c..77818e131 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -802,17 +802,7 @@ void parser_t::get_backtrace(const wcstring &src, const parse_error_list_t &erro } } -block_t::block_t(block_type_t t) - : block_type(t), - skip(false), - tok_pos(), - node_offset(NODE_OFFSET_INVALID), - loop_status(LOOP_NORMAL), - job(), - src_filename(), - src_lineno(), - wants_pop_env(false), - event_blocks() {} +block_t::block_t(block_type_t t) : block_type(t) {} block_t::~block_t() {} diff --git a/src/parser.h b/src/parser.h index edb51dcf6..7c061a2c3 100644 --- a/src/parser.h +++ b/src/parser.h @@ -75,31 +75,26 @@ struct block_t { public: /// Whether execution of the commands in this block should be skipped. - bool skip; - /// The start index of the block. - int tok_pos; - /// Offset of the node. - node_offset_t node_offset; + bool skip{false}; /// Status for the current loop block. Can be any of the values from the loop_status enum. - enum loop_status_t loop_status; + enum loop_status_t loop_status { LOOP_NORMAL }; /// The job that is currently evaluated in the specified block. - shared_ptr job; + shared_ptr job{}; /// Name of file that created this block. This string is intern'd. - const wchar_t *src_filename; + const wchar_t *src_filename{nullptr}; /// Line number where this block was created. - int src_lineno; + int src_lineno{0}; /// Whether we should pop the environment variable stack when we're popped off of the block /// stack. - bool wants_pop_env; + bool wants_pop_env{false}; + /// List of event blocks. + event_blockage_list_t event_blocks{}; block_type_t type() const { return this->block_type; } /// Description of the block, for debugging. wcstring description() const; - /// List of event blocks. - event_blockage_list_t event_blocks; - /// Destructor virtual ~block_t(); }; From 8bddce26330ad3689dfdc2da0903bedf7a8aeb7e Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 11 Feb 2018 20:17:34 -0800 Subject: [PATCH 10/14] Remove fake_block_t It was unused. --- src/parser.h | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/parser.h b/src/parser.h index 7c061a2c3..8be14ef2d 100644 --- a/src/parser.h +++ b/src/parser.h @@ -131,10 +131,6 @@ struct switch_block_t : public block_t { switch_block_t(); }; -struct fake_block_t : public block_t { - fake_block_t(); -}; - struct scope_block_t : public block_t { explicit scope_block_t(block_type_t type); // must be BEGIN, TOP or SUBST }; From 92d5c28730e101297f68a188ffe3d9e8b43eea71 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 11 Feb 2018 21:42:23 -0800 Subject: [PATCH 11/14] Move eval_level into parser_t This avoids the ping-ponging of eval_level through parse_execution_context. Simply store the global eval level in the parser_t. --- src/parse_execution.cpp | 11 +++++------ src/parse_execution.h | 7 +------ src/parser.cpp | 9 +-------- src/parser.h | 2 ++ 4 files changed, 9 insertions(+), 20 deletions(-) diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 1c6741df5..70b62fd9e 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -79,9 +79,8 @@ static wcstring profiling_cmd_name_for_redirectable_block(const parse_node_t &no return result; } -parse_execution_context_t::parse_execution_context_t(parsed_source_ref_t pstree, parser_t *p, - int initial_eval_level) - : pstree(std::move(pstree)), parser(p), eval_level(initial_eval_level) {} +parse_execution_context_t::parse_execution_context_t(parsed_source_ref_t pstree, parser_t *p) + : pstree(std::move(pstree)), parser(p) {} // Utilities @@ -1115,7 +1114,7 @@ parse_execution_result_t parse_execution_context_t::run_1_job(tnode_t jo } // Increment the eval_level for the duration of this command. - scoped_push saved_eval_level(&eval_level, eval_level + 1); + scoped_push saved_eval_level(&parser->eval_level, parser->eval_level + 1); // Save the node index. scoped_push> saved_node(&executing_job_node, job_node); @@ -1162,7 +1161,7 @@ parse_execution_result_t parse_execution_context_t::run_1_job(tnode_t jo // Block-types profile a little weird. They have no 'parse' time, and their command is // just the block type. exec_time = get_time(); - profile_item->level = eval_level; + profile_item->level = parser->eval_level; profile_item->parse = 0; profile_item->exec = (int)(exec_time - start_time); profile_item->cmd = profiling_cmd_name_for_redirectable_block( @@ -1231,7 +1230,7 @@ parse_execution_result_t parse_execution_context_t::run_1_job(tnode_t jo if (profile_item != NULL) { exec_time = get_time(); - profile_item->level = eval_level; + profile_item->level = parser->eval_level; profile_item->parse = (int)(parse_time - start_time); profile_item->exec = (int)(exec_time - parse_time); profile_item->cmd = job ? job->command() : wcstring(); diff --git a/src/parse_execution.h b/src/parse_execution.h index 7b99691a9..5b469ab04 100644 --- a/src/parse_execution.h +++ b/src/parse_execution.h @@ -31,8 +31,6 @@ class parse_execution_context_t { parsed_source_ref_t pstree; io_chain_t block_io; parser_t *const parser; - // parse_error_list_t errors; - int eval_level; // The currently executing job node, used to indicate the line number. tnode_t executing_job_node{}; // Cached line number information. @@ -128,10 +126,7 @@ class parse_execution_context_t { int line_offset_of_character_at_offset(size_t char_idx); public: - parse_execution_context_t(parsed_source_ref_t pstree, parser_t *p, int initial_eval_level); - - /// Returns the current eval level. - int current_eval_level() const { return eval_level; } + parse_execution_context_t(parsed_source_ref_t pstree, parser_t *p); /// Returns the current line number, indexed from 1. Not const since it touches /// cached_lineno_offset. diff --git a/src/parser.cpp b/src/parser.cpp index 77818e131..38804af9d 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -695,15 +695,8 @@ int parser_t::eval_node(parsed_source_ref_t ps, tnode_t node, const io_chain_ // Start it up scope_block_t *scope_block = this->push_block(block_type); - // Determine the initial eval level. If this is the first context, it's -1; otherwise it's the - // eval level of the top context. This is sort of wonky because we're stitching together a - // global notion of eval level from these separate objects. A better approach would be some - // profile object that all contexts share, and that tracks the eval levels on its own. - int exec_eval_level = - (execution_contexts.empty() ? -1 : execution_contexts.back()->current_eval_level()); - // Append to the execution context stack. - execution_contexts.push_back(make_unique(ps, this, exec_eval_level)); + execution_contexts.push_back(make_unique(ps, this)); parse_execution_context_t *ctx = execution_contexts.back().get(); int result = ctx->eval_node(node, scope_block, io); diff --git a/src/parser.h b/src/parser.h index 8be14ef2d..57a6c2a2c 100644 --- a/src/parser.h +++ b/src/parser.h @@ -185,6 +185,8 @@ class parser_t { job_list_t my_job_list; /// The list of blocks std::vector> block_stack; + /// The 'depth' of the fish call stack. + int eval_level = -1; #if 0 // TODO: Lint says this isn't used (which is true). Should this be removed? From 4eb73862fc76f27039be2df755540aed48f49dcc Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 11 Feb 2018 22:00:17 -0800 Subject: [PATCH 12/14] Switch parser_t::execution_contexts to only storing one We only ever need the topmost (currently executing) context. We can store the stack via the C stack rather than an explicit std::vector. --- src/parser.cpp | 30 ++++++++++++------------------ src/parser.h | 4 ++-- 2 files changed, 14 insertions(+), 20 deletions(-) diff --git a/src/parser.cpp b/src/parser.cpp index 38804af9d..e44744e24 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -483,8 +483,8 @@ const wchar_t *parser_t::get_function_name(int level) { int parser_t::get_lineno() const { int lineno = -1; - if (!execution_contexts.empty()) { - lineno = execution_contexts.back()->get_current_line_number(); + if (execution_context) { + lineno = execution_context->get_current_line_number(); // If we are executing a function, we have to add in its offset. const wchar_t *function_name = is_function(); @@ -518,13 +518,10 @@ const wchar_t *parser_t::current_filename() const { } wcstring parser_t::current_line() { - if (execution_contexts.empty()) { + if (!execution_context) { return wcstring(); } - const parse_execution_context_t *context = execution_contexts.back().get(); - assert(context != NULL); - - int source_offset = context->get_current_source_offset(); + int source_offset = execution_context->get_current_source_offset(); if (source_offset < 0) { return wcstring(); } @@ -554,8 +551,8 @@ wcstring parser_t::current_line() { parse_error_t empty_error = {}; empty_error.source_start = source_offset; - wcstring line_info = - empty_error.describe_with_prefix(context->get_source(), prefix, is_interactive, skip_caret); + wcstring line_info = empty_error.describe_with_prefix(execution_context->get_source(), prefix, + is_interactive, skip_caret); if (!line_info.empty()) { line_info.push_back(L'\n'); } @@ -695,17 +692,14 @@ int parser_t::eval_node(parsed_source_ref_t ps, tnode_t node, const io_chain_ // Start it up scope_block_t *scope_block = this->push_block(block_type); - // Append to the execution context stack. - execution_contexts.push_back(make_unique(ps, this)); - parse_execution_context_t *ctx = execution_contexts.back().get(); - - int result = ctx->eval_node(node, scope_block, io); + // Create and set a new execution context. + using exc_ctx_ref_t = std::unique_ptr; + scoped_push exc(&execution_context, + make_unique(ps, this)); + int result = execution_context->eval_node(node, scope_block, io); + exc.restore(); this->pop_block(scope_block); - // Clean up the execution context stack. - assert(!execution_contexts.empty() && execution_contexts.back().get() == ctx); - execution_contexts.pop_back(); - job_reap(0); // reap again return result; } diff --git a/src/parser.h b/src/parser.h index 57a6c2a2c..289f8b8ae 100644 --- a/src/parser.h +++ b/src/parser.h @@ -177,8 +177,8 @@ class parser_t { volatile sig_atomic_t cancellation_requested; /// Indicates that we are within the process of initializing fish. bool is_within_fish_initialization; - /// Stack of execution contexts. - std::vector> execution_contexts; + /// The current execution context. + std::unique_ptr execution_context; /// List of called functions, used to help prevent infinite recursion. wcstring_list_t forbidden_function; /// The jobs associated with this parser. From d536b152f733525f44362b22ee656b9298f69aa2 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 11 Feb 2018 23:13:06 -0800 Subject: [PATCH 13/14] Simplify the parser_t::eval() return type to void The return value was unused. --- src/parser.cpp | 18 +++++++----------- src/parser.h | 4 ++-- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/src/parser.cpp b/src/parser.cpp index e44744e24..92c33b706 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -644,22 +644,18 @@ int parser_t::eval(wcstring cmd, const io_chain_t &io, enum block_type_t block_t fwprintf(stderr, L"%ls\n", backtrace_and_desc.c_str()); return 1; } - return this->eval(ps, io, block_type); + this->eval(ps, io, block_type); + return 0; } -int parser_t::eval(parsed_source_ref_t ps, const io_chain_t &io, enum block_type_t block_type) { +void parser_t::eval(parsed_source_ref_t ps, const io_chain_t &io, enum block_type_t block_type) { CHECK_BLOCK(1); assert(block_type == TOP || block_type == SUBST); - - if (ps->tree.empty()) { - return 0; + if (!ps->tree.empty()) { + // Execute the first node. + tnode_t start{&ps->tree, &ps->tree.front()}; + this->eval_node(ps, start, io, block_type); } - - // Execute the first node. - tnode_t start{&ps->tree, &ps->tree.front()}; - this->eval_node(ps, start, io, block_type); - - return 0; } template diff --git a/src/parser.h b/src/parser.h index 289f8b8ae..35ca8922f 100644 --- a/src/parser.h +++ b/src/parser.h @@ -238,11 +238,11 @@ class parser_t { /// \param io io redirections to perform on all started jobs /// \param block_type The type of block to push on the block stack /// - /// \return 0 on success, 1 otherwise + /// \return 0 on success, 1 on a parse error. int eval(wcstring cmd, const io_chain_t &io, enum block_type_t block_type); /// Evaluate the parsed source ps. - int eval(parsed_source_ref_t ps, const io_chain_t &io, enum block_type_t block_type); + void eval(parsed_source_ref_t ps, const io_chain_t &io, enum block_type_t block_type); /// Evaluates a node. /// The node type must be grammar::statement or grammar::job_list. From 5b5a3f78e18bdb72ed4fdd45c8ac751c83c3761e Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Mon, 12 Feb 2018 01:18:11 -0800 Subject: [PATCH 14/14] Eliminate parse_execution_context_t::get_offset This was used to find the index given a pointer to a node. It's now unused. --- src/parse_execution.cpp | 11 ----------- src/parse_execution.h | 1 - 2 files changed, 12 deletions(-) diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 70b62fd9e..e1ff43dc6 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -88,17 +88,6 @@ wcstring parse_execution_context_t::get_source(const parse_node_t &node) const { return node.get_source(pstree->src); } -node_offset_t parse_execution_context_t::get_offset(const parse_node_t &node) const { - // Get the offset of a node via pointer arithmetic, very hackish. - const parse_node_t *addr = &node; - const parse_node_t *base = &this->tree().at(0); - assert(addr >= base); - node_offset_t offset = static_cast(addr - base); - assert(offset < this->tree().size()); - assert(&tree().at(offset) == &node); - return offset; -} - tnode_t parse_execution_context_t::infinite_recursive_statement_in_job_list( tnode_t job_list, wcstring *out_func_name) const { // This is a bit fragile. It is a test to see if we are inside of function call, but not inside diff --git a/src/parse_execution.h b/src/parse_execution.h index 5b469ab04..403d4392f 100644 --- a/src/parse_execution.h +++ b/src/parse_execution.h @@ -68,7 +68,6 @@ class parse_execution_context_t { // Utilities wcstring get_source(const parse_node_t &node) const; - node_offset_t get_offset(const parse_node_t &node) const; tnode_t infinite_recursive_statement_in_job_list( tnode_t job_list, wcstring *out_func_name) const; bool is_function_context() const;