From 194f7f34d96e22fafa0636e9eb8d71d63a3b073f Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Mon, 15 Jan 2018 22:13:37 -0800 Subject: [PATCH] Eliminate parse_node_tree::find_nodes --- src/complete.cpp | 2 +- src/fish_indent.cpp | 9 +++--- src/fish_tests.cpp | 65 ++++++++++++++++++++--------------------- src/parse_execution.cpp | 10 ++----- src/parse_grammar.h | 7 +++++ src/parse_tree.cpp | 36 +++++------------------ src/parse_tree.h | 42 +++++++++++--------------- src/parse_util.cpp | 20 ++++++------- 8 files changed, 80 insertions(+), 111 deletions(-) diff --git a/src/complete.cpp b/src/complete.cpp index 5060e1ed1..b61d6ba1f 100644 --- a/src/complete.cpp +++ b/src/complete.cpp @@ -1367,7 +1367,7 @@ void complete(const wcstring &cmd_with_subcmds, std::vector *out_c use_implicit_cd); } else { // Get all the arguments. - auto all_arguments = tree.find_nodes(plain_statement); + auto all_arguments = plain_statement.descendants(); // See whether we are in an argument. We may also be in a redirection, or nothing at // all. diff --git a/src/fish_indent.cpp b/src/fish_indent.cpp index bc33a9e72..ca1ce7c47 100644 --- a/src/fish_indent.cpp +++ b/src/fish_indent.cpp @@ -132,12 +132,11 @@ static void prettify_node_recursive(const wcstring &source, const parse_node_tre if (node.has_comments()) // handle comments, which come before the text { - const parse_node_tree_t::parse_node_list_t comment_nodes = - (tree.comment_nodes_for_node(node)); - for (size_t i = 0; i < comment_nodes.size(); i++) { - const parse_node_t &comment_node = *comment_nodes.at(i); + auto comment_nodes = tree.comment_nodes_for_node(node); + for (const auto &comment : comment_nodes) { append_whitespace(node_indent, do_indent, *has_new_line, out_result); - out_result->append(source, comment_node.source_start, comment_node.source_length); + auto source_range = comment.source_range(); + out_result->append(source, source_range->start, source_range->length); } } diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 6ce192837..e9946e756 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -2418,7 +2418,6 @@ static void test_autosuggest_suggest_special() { } const wcstring wd = L"test/autosuggest_test"; - const env_vars_snapshot_t &vars = env_vars_snapshot_t::current(); perform_one_autosuggestion_cd_test(L"cd test/autosuggest_test/0", L"foobar/", __LINE__); perform_one_autosuggestion_cd_test(L"cd \"test/autosuggest_test/0", L"foobar/", __LINE__); @@ -3394,7 +3393,8 @@ static bool test_1_parse_ll2(const wcstring &src, wcstring *out_cmd, wcstring *o } // Get the statement. Should only have one. - auto stmts = tree.find_nodes(tree.at(0)); + tnode_t job_list{&tree, &tree.at(0)}; + auto stmts = job_list.descendants(); if (stmts.size() != 1) { say(L"Unexpected number of statements (%lu) found in '%ls'", stmts.size(), src.c_str()); return false; @@ -3406,15 +3406,34 @@ static bool test_1_parse_ll2(const wcstring &src, wcstring *out_cmd, wcstring *o *out_cmd = *command_for_plain_statement(stmt, src); // Return arguments separated by spaces. - const parse_node_tree_t::parse_node_list_t arg_nodes = tree.find_nodes(stmt, symbol_argument); - for (size_t i = 0; i < arg_nodes.size(); i++) { - if (i > 0) out_joined_args->push_back(L' '); - out_joined_args->append(arg_nodes.at(i)->get_source(src)); + bool first = true; + for (auto arg_node : stmt.descendants()) { + if (!first) out_joined_args->push_back(L' '); + out_joined_args->append(arg_node.get_source(src)); + first = false; } return true; } +// Verify that 'function -h' and 'function --help' are plain statements but 'function --foo' is +// not (issue #1240). +template +static void check_function_help(const wchar_t *src) { + parse_node_tree_t tree; + if (!parse_tree_from_string(src, parse_flag_none, &tree, NULL)) { + err(L"Failed to parse '%ls'", src); + } + + tnode_t node{&tree, &tree.at(0)}; + auto node_list = node.descendants(); + if (node_list.size() == 0) { + err(L"Failed to find node of type '%ls'", token_type_description(Type::token)); + } else if (node_list.size() > 1) { + err(L"Found too many nodes of type '%ls'", token_type_description(Type::token)); + } +} + // Test the LL2 (two token lookahead) nature of the parser by exercising the special builtin and // command handling. In particular, 'command foo' should be a decorated statement 'foo' but 'command // -help' should be an undecorated statement 'command' with argument '--help', and NOT attempt to @@ -3459,31 +3478,10 @@ static void test_new_parser_ll2(void) { tests[i].src.c_str(), (int)tests[i].deco, (int)deco, (long)__LINE__); } - // Verify that 'function -h' and 'function --help' are plain statements but 'function --foo' is - // not (issue #1240). - const struct { - wcstring src; - parse_token_type_t type; - } tests2[] = { - {L"function -h", symbol_plain_statement}, - {L"function --help", symbol_plain_statement}, - {L"function --foo ; end", symbol_function_header}, - {L"function foo ; end", symbol_function_header}, - }; - for (size_t i = 0; i < sizeof tests2 / sizeof *tests2; i++) { - parse_node_tree_t tree; - if (!parse_tree_from_string(tests2[i].src, parse_flag_none, &tree, NULL)) { - err(L"Failed to parse '%ls'", tests2[i].src.c_str()); - } - - const parse_node_tree_t::parse_node_list_t node_list = - tree.find_nodes(tree.at(0), tests2[i].type); - if (node_list.size() == 0) { - err(L"Failed to find node of type '%ls'", token_type_description(tests2[i].type)); - } else if (node_list.size() > 1) { - err(L"Found too many nodes of type '%ls'", token_type_description(tests2[i].type)); - } - } + check_function_help(L"function -h"); + check_function_help(L"function --help"); + check_function_help(L"function --foo; end"); + check_function_help(L"function foo; end"); } static void test_new_parser_ad_hoc() { @@ -3500,9 +3498,8 @@ static void test_new_parser_ad_hoc() { // Expect three case_item_lists: one for each case, and a terminal one. The bug was that we'd // try to run a command 'case'. - const parse_node_t &root = parse_tree.at(0); - const parse_node_tree_t::parse_node_list_t node_list = - parse_tree.find_nodes(root, symbol_case_item_list); + tnode_t root{&parse_tree, &parse_tree.at(0)}; + auto node_list = root.descendants(); if (node_list.size() != 3) { err(L"Expected 3 case item nodes, found %lu", node_list.size()); } diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 72bebf415..50f692503 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -66,13 +66,9 @@ static wcstring profiling_cmd_name_for_redirectable_block(const parse_node_t &no const size_t src_start = node.source_start; size_t src_len = node.source_length; - const parse_node_tree_t::parse_node_list_t statement_terminator_nodes = - tree.find_nodes(node, parse_token_type_end, 1); - if (!statement_terminator_nodes.empty()) { - const parse_node_t *term = statement_terminator_nodes.at(0); - assert(term->source_start >= src_start); - src_len = term->source_start - src_start; - } + auto term = tree.find_child(node); + assert(term.has_source() && term.source_range()->start >= src_start); + src_len = term.source_range()->start - src_start; wcstring result = wcstring(src, src_start, src_len); result.append(L"..."); diff --git a/src/parse_grammar.h b/src/parse_grammar.h index cd82a5ecf..972b22f1a 100644 --- a/src/parse_grammar.h +++ b/src/parse_grammar.h @@ -42,6 +42,13 @@ struct keyword { } }; +// Define special types. +// Comments are not emitted as part of productions, but specially by the parser. +struct comment { + using type_tuple = std::tuple<>; + static constexpr parse_token_type_t token = parse_special_type_comment; +}; + // Forward declare all the symbol types. #define ELEM(T) struct T; #include "parse_grammar_elements.inc" diff --git a/src/parse_tree.cpp b/src/parse_tree.cpp index a9e0e48c5..64008d77c 100644 --- a/src/parse_tree.cpp +++ b/src/parse_tree.cpp @@ -1231,27 +1231,6 @@ const parse_node_t *parse_node_tree_t::get_parent(const parse_node_t &node, return result; } -static void find_nodes_recursive(const parse_node_tree_t &tree, const parse_node_t &parent, - parse_token_type_t type, - parse_node_tree_t::parse_node_list_t *result, size_t max_count) { - if (result->size() < max_count) { - if (parent.type == type) result->push_back(&parent); - for (node_offset_t i = 0; i < parent.child_count; i++) { - const parse_node_t *child = tree.get_child(parent, i); - assert(child != NULL); - find_nodes_recursive(tree, *child, type, result, max_count); - } - } -} - -parse_node_tree_t::parse_node_list_t parse_node_tree_t::find_nodes(const parse_node_t &parent, - parse_token_type_t type, - size_t max_count) const { - parse_node_list_t result; - find_nodes_recursive(*this, parent, type, &result, max_count); - return result; -} - /// Return true if the given node has the proposed ancestor as an ancestor (or is itself that /// ancestor). static bool node_has_ancestor(const parse_node_tree_t &tree, const parse_node_t &node, @@ -1365,16 +1344,16 @@ bool parse_node_tree_t::statement_is_in_pipeline(const parse_node_t &node, return result; } -parse_node_tree_t::parse_node_list_t parse_node_tree_t::comment_nodes_for_node( +std::vector> parse_node_tree_t::comment_nodes_for_node( const parse_node_t &parent) const { - parse_node_list_t result; + std::vector> result; if (parent.has_comments()) { // Walk all our nodes, looking for comment nodes that have the given node as a parent. for (size_t i = 0; i < this->size(); i++) { const parse_node_t &potential_comment = this->at(i); if (potential_comment.type == parse_special_type_comment && this->get_parent(potential_comment) == &parent) { - result.push_back(&potential_comment); + result.emplace_back(this, &potential_comment); } } } @@ -1428,12 +1407,13 @@ maybe_t command_for_plain_statement(tnode_t return none(); } -arguments_node_list_t get_argument_nodes(tnode_t list) { - return list.descendants(); +arguments_node_list_t get_argument_nodes(tnode_t list, size_t max) { + return list.descendants(max); } -arguments_node_list_t get_argument_nodes(tnode_t list) { - return list.descendants(); +arguments_node_list_t get_argument_nodes(tnode_t list, + size_t max) { + return list.descendants(max); } bool job_node_is_background(tnode_t job) { diff --git a/src/parse_tree.h b/src/parse_tree.h index 898714070..5855ec89a 100644 --- a/src/parse_tree.h +++ b/src/parse_tree.h @@ -169,15 +169,6 @@ class parse_node_tree_t : public std::vector { const parse_node_t *get_parent(const parse_node_t &node, parse_token_type_t expected_type = token_type_invalid) const; - // Find all the nodes of a given type underneath a given node, up to max_count of them. - typedef std::vector parse_node_list_t; - parse_node_list_t find_nodes(const parse_node_t &parent, parse_token_type_t type, - size_t max_count = size_t(-1)) const; - - // Find all the nodes of a given type underneath a given node, up to max_count of them. - template - std::vector> find_nodes(const parse_node_t &parent, size_t max_count = -1) const; - // Finds the last node of a given type, or empty if it could not be found. If parent is NULL, // this finds the last node in the tree of that type. template @@ -196,7 +187,7 @@ class parse_node_tree_t : public std::vector { bool statement_is_in_pipeline(const parse_node_t &node, bool include_first) const; /// Given a node, return all of its comment nodes. - parse_node_list_t comment_nodes_for_node(const parse_node_t &node) const; + std::vector> comment_nodes_for_node(const parse_node_t &node) const; private: template @@ -377,7 +368,18 @@ class tnode_t { template std::vector> descendants(size_t max_count = -1) const { if (!nodeptr) return {}; - return tree->find_nodes(*nodeptr); + std::vector> result; + std::vector stack{nodeptr}; + while (!stack.empty() && result.size() < max_count) { + const parse_node_t *node = stack.back(); + if (node->type == DescendantType::token) result.emplace_back(tree, node); + stack.pop_back(); + node_offset_t index = node->child_count; + while (index--) { + stack.push_back(tree->get_child(*node, index)); + } + } + return result; } /// Given that we are a list type, \return the next node of some Item in some node list, @@ -402,18 +404,6 @@ tnode_t parse_node_tree_t::find_last_node(const parse_node_t *parent) cons return tnode_t(this, this->find_last_node_of_type(Type::token, parent)); } -template -std::vector> parse_node_tree_t::find_nodes(const parse_node_t &parent, - size_t max_count) const { - auto ptrs = this->find_nodes(parent, Type::token, max_count); - std::vector> result; - result.reserve(ptrs.size()); - for (const parse_node_t *np : ptrs) { - result.emplace_back(this, np); - } - return result; -} - /// Given a plain statement, get the command from the child node. Returns the command string on /// success, none on failure. maybe_t command_for_plain_statement(tnode_t stmt, @@ -430,9 +420,11 @@ enum token_type redirection_type(tnode_t redirection, cons int *out_fd, wcstring *out_target); /// Return the arguments under an arguments_list or arguments_or_redirection_list +/// Do not return more than max. using arguments_node_list_t = std::vector>; -arguments_node_list_t get_argument_nodes(tnode_t); -arguments_node_list_t get_argument_nodes(tnode_t); +arguments_node_list_t get_argument_nodes(tnode_t, size_t max = -1); +arguments_node_list_t get_argument_nodes(tnode_t, + size_t max = -1); /// Return whether the given job is background because it has a & symbol. bool job_node_is_background(tnode_t); diff --git a/src/parse_util.cpp b/src/parse_util.cpp index 081617676..5cfada44f 100644 --- a/src/parse_util.cpp +++ b/src/parse_util.cpp @@ -760,15 +760,13 @@ bool parse_util_argument_is_help(const wchar_t *s) { } /// Check if the first argument under the given node is --help. -static bool first_argument_is_help(const parse_node_tree_t &node_tree, const parse_node_t &node, +static bool first_argument_is_help(tnode_t statement, const wcstring &src) { bool is_help = false; - const parse_node_tree_t::parse_node_list_t arg_nodes = - node_tree.find_nodes(node, symbol_argument, 1); + auto arg_nodes = get_argument_nodes(statement.child<1>()); if (!arg_nodes.empty()) { // Check the first argument only. - const parse_node_t &arg = *arg_nodes.at(0); - const wcstring first_arg_src = arg.get_source(src); + wcstring first_arg_src = arg_nodes.front().get_source(src); is_help = parse_util_argument_is_help(first_arg_src.c_str()); } return is_help; @@ -1167,13 +1165,14 @@ parser_test_error_bits_t parse_util_detect_errors(const wcstring &buff_src, errored |= detect_errors_in_backgrounded_job(node_tree, job, &parse_errors); } } else if (node.type == symbol_plain_statement) { + using namespace grammar; + tnode_t statement{&node_tree, &node}; // In a few places below, we want to know if we are in a pipeline. const bool is_in_pipeline = - node_tree.statement_is_in_pipeline(node, true /* count first */); + node_tree.statement_is_in_pipeline(statement, true /* count first */); // We need to know the decoration. - const enum parse_statement_decoration_t decoration = - get_decoration({&node_tree, &node}); + const enum parse_statement_decoration_t decoration = get_decoration(statement); // Check that we don't try to pipe through exec. if (is_in_pipeline && decoration == parse_statement_decoration_exec) { @@ -1183,7 +1182,6 @@ parser_test_error_bits_t parse_util_detect_errors(const wcstring &buff_src, if (maybe_t mcommand = command_for_plain_statement({&node_tree, &node}, buff_src)) { - using namespace grammar; wcstring command = std::move(*mcommand); // Check that we can expand the command. if (!expand_one(command, @@ -1214,7 +1212,7 @@ parser_test_error_bits_t parse_util_detect_errors(const wcstring &buff_src, break; } } - if (!found_function && !first_argument_is_help(node_tree, node, buff_src)) { + if (!found_function && !first_argument_is_help(statement, buff_src)) { errored = append_syntax_error(&parse_errors, node.source_start, INVALID_RETURN_ERR_MSG); } @@ -1246,7 +1244,7 @@ parser_test_error_bits_t parse_util_detect_errors(const wcstring &buff_src, } } - if (!found_loop && !first_argument_is_help(node_tree, node, buff_src)) { + if (!found_loop && !first_argument_is_help(statement, buff_src)) { errored = append_syntax_error( &parse_errors, node.source_start, (command == L"break" ? INVALID_BREAK_ERR_MSG