From 4529e7d1836bbd3e85301123ad7506c792602e0c Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 22 Dec 2019 15:07:41 -0800 Subject: [PATCH] Reverse the order of the block stack Previously, the block stack was a true stack. However in most cases, you want to traverse the stack from the topmost frame down. This is awkward to do with range-based for loops. Switch it to pushing new blocks to the front of the block list. This simplifies some traversals. --- src/builtin.cpp | 10 +- src/builtin_block.cpp | 2 +- src/builtin_return.cpp | 12 ++- src/parser.cpp | 239 +++++++++++++++++++---------------------- src/parser.h | 20 ++-- src/reader.cpp | 4 +- src/trace.cpp | 2 +- 7 files changed, 141 insertions(+), 148 deletions(-) diff --git a/src/builtin.cpp b/src/builtin.cpp index 0a09fca33..b727fba4c 100644 --- a/src/builtin.cpp +++ b/src/builtin.cpp @@ -264,10 +264,12 @@ static int builtin_break_continue(parser_t &parser, io_streams_t &streams, wchar // Paranoia: ensure we have a real loop. bool has_loop = false; - for (size_t loop_idx = 0; loop_idx < parser.block_count() && !has_loop; loop_idx++) { - const block_t *b = parser.block_at_index(loop_idx); - if (b->type() == WHILE || b->type() == FOR) has_loop = true; - if (b->type() == FUNCTION_CALL) break; + for (const auto &b : parser.blocks()) { + if (b.type() == WHILE || b.type() == FOR) { + has_loop = true; + break; + } + if (b.is_function()) break; } if (!has_loop) { wcstring error_message = format_string(_(L"%ls: Not inside of loop\n"), argv[0]); diff --git a/src/builtin_block.cpp b/src/builtin_block.cpp index d97ceecf2..73509e8cb 100644 --- a/src/builtin_block.cpp +++ b/src/builtin_block.cpp @@ -107,7 +107,7 @@ int builtin_block(parser_t &parser, io_streams_t &streams, wchar_t **argv) { switch (opts.scope) { case LOCAL: { // If this is the outermost block, then we're global - if (block_idx + 1 >= parser.block_count()) { + if (block_idx + 1 >= parser.blocks().size()) { block = nullptr; } break; diff --git a/src/builtin_return.cpp b/src/builtin_return.cpp index 579ca5835..7ef2e5f7f 100644 --- a/src/builtin_return.cpp +++ b/src/builtin_return.cpp @@ -91,13 +91,15 @@ int builtin_return(parser_t &parser, io_streams_t &streams, wchar_t **argv) { } // Find the function block. - size_t function_block_idx; - for (function_block_idx = 0; function_block_idx < parser.block_count(); function_block_idx++) { - const block_t *b = parser.block_at_index(function_block_idx); - if (b->type() == FUNCTION_CALL || b->type() == FUNCTION_CALL_NO_SHADOW) break; + bool has_function_block = false; + for (const auto &b : parser.blocks()) { + if (b.is_function()) { + has_function_block = true; + break; + } } - if (function_block_idx >= parser.block_count()) { + if (!has_function_block) { streams.err.append_format(_(L"%ls: Not inside of function\n"), cmd); builtin_print_error_trailer(parser, streams.err, cmd); return STATUS_CMD_ERROR; diff --git a/src/parser.cpp b/src/parser.cpp index 20fa4e01e..d4c700db6 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -16,6 +16,7 @@ #include "event.h" #include "expand.h" #include "fallback.h" // IWYU pragma: keep +#include "flog.h" #include "function.h" #include "intern.h" #include "parse_constants.h" @@ -134,12 +135,14 @@ void parser_t::skip_all_blocks() { principal->cancellation_requested = true; } -// Given a new-allocated block, push it onto our block stack, acquiring ownership +// Given a new-allocated block, push it onto our block list, acquiring ownership. block_t *parser_t::push_block(block_t &&block) { block_t new_current{std::move(block)}; const enum block_type_t type = new_current.type(); new_current.src_lineno = parser_t::get_lineno(); + wcstring func = new_current.function_name; + const wchar_t *filename = parser_t::current_filename(); if (filename != nullptr) { new_current.src_filename = intern(filename); @@ -159,31 +162,26 @@ block_t *parser_t::push_block(block_t &&block) { new_current.wants_pop_env = true; } - // Push it onto our stack and return a pointer to it. + // Push it onto our list and return a pointer to it. // Note that deques do not move their contents so this is safe. - this->block_stack.push_back(std::move(new_current)); - return &this->block_stack.back(); + this->block_list.push_front(std::move(new_current)); + return &this->block_list.front(); } void parser_t::pop_block(const block_t *expected) { assert(expected == this->current_block()); - if (block_stack.empty()) { - debug(1, L"function %s called on empty block stack.", __func__); - bugreport(); - return; - } + assert(!block_list.empty() && "empty block list"); - // Acquire ownership out of the block stack. - block_t old = std::move(block_stack.back()); - block_stack.pop_back(); + // Acquire ownership out of the block list. + block_t old = std::move(block_list.front()); + block_list.pop_front(); if (old.wants_pop_env) vars().pop(); // Figure out if `status is-block` should consider us to be in a block now. bool new_is_block = false; - for (const auto &b : block_stack) { - const enum block_type_t type = b.type(); - if (type != TOP && type != SUBST) { + for (const auto &b : block_list) { + if (b.type() != TOP && b.type() != SUBST) { new_is_block = true; break; } @@ -192,9 +190,8 @@ void parser_t::pop_block(const block_t *expected) { // Are we still in a breakpoint? bool new_is_breakpoint = false; - for (const auto &b : block_stack) { - const enum block_type_t type = b.type(); - if (type == BREAKPOINT) { + for (const auto &b : block_list) { + if (b.type() == BREAKPOINT) { new_is_breakpoint = true; break; } @@ -232,17 +229,14 @@ wcstring parser_t::block_stack_description() const { #endif const block_t *parser_t::block_at_index(size_t idx) const { - // Zero corresponds to the last element in our vector. - size_t count = block_stack.size(); - return idx < count ? &block_stack.at(count - idx - 1) : nullptr; + return idx < block_list.size() ? &block_list[idx] : nullptr; } block_t *parser_t::block_at_index(size_t idx) { - size_t count = block_stack.size(); - return idx < count ? &block_stack.at(count - idx - 1) : nullptr; + return idx < block_list.size() ? &block_list[idx] : nullptr; } -block_t *parser_t::current_block() { return block_stack.empty() ? nullptr : &block_stack.back(); } +block_t *parser_t::current_block() { return block_at_index(0); } /// Print profiling information to the specified stream. static void print_profile(const std::vector> &items, FILE *out) { @@ -340,90 +334,82 @@ std::vector parser_t::expand_argument_list(const wcstring &arg_lis return result; } -wcstring parser_t::stack_trace() const { - wcstring trace; - this->stack_trace_internal(0, &trace); - return trace; -} - std::shared_ptr parser_t::shared() { return shared_from_this(); } -void parser_t::stack_trace_internal(size_t block_idx, wcstring *buff) const { - // Check if we should end the recursion. - if (block_idx >= this->block_count()) return; +wcstring parser_t::stack_trace() const { + wcstring trace; + for (const auto &b : blocks()) { + if (b.type() == EVENT) { + // This is an event handler. + assert(b.event && "Should have an event"); + wcstring description = event_get_desc(*b.event); + append_format(trace, _(L"in event handler: %ls\n"), description.c_str()); - const block_t *b = this->block_at_index(block_idx); + // Stop at event handler. No reason to believe that any other code is relevant. + // + // It might make sense in the future to continue printing the stack trace of the code + // that invoked the event, if this is a programmatic event, but we can't currently + // detect that. + break; + } - if (b->type() == EVENT) { - // This is an event handler. - assert(b->event && "Should have an event"); - wcstring description = event_get_desc(*b->event); - append_format(*buff, _(L"in event handler: %ls\n"), description.c_str()); - - // Stop recursing at event handler. No reason to believe that any other code is relevant. - // - // It might make sense in the future to continue printing the stack trace of the code that - // invoked the event, if this is a programmatic event, but we can't currently detect that. - return; - } - - if (b->type() == FUNCTION_CALL || b->type() == FUNCTION_CALL_NO_SHADOW || b->type() == SOURCE || - b->type() == SUBST) { - // These types of blocks should be printed. - switch (b->type()) { - case SOURCE: { - const wchar_t *source_dest = b->sourced_file; - append_format(*buff, _(L"from sourcing file %ls\n"), - user_presentable_path(source_dest).c_str()); - break; - } - case FUNCTION_CALL: - case FUNCTION_CALL_NO_SHADOW: { - append_format(*buff, _(L"in function '%ls'"), b->function_name.c_str()); - // Print arguments on the same line. - wcstring args_str; - for (const wcstring &arg : b->function_args) { - if (!args_str.empty()) args_str.push_back(L' '); - // We can't quote the arguments because we print this in quotes. - // As a special-case, add the empty argument as "". - if (!arg.empty()) { - args_str.append(escape_string(arg, ESCAPE_ALL | ESCAPE_NO_QUOTED)); - } else { - args_str.append(L"\"\""); + if (b.type() == FUNCTION_CALL || b.type() == FUNCTION_CALL_NO_SHADOW || + b.type() == SOURCE || b.type() == SUBST) { + // These types of blocks should be printed. + switch (b.type()) { + case SOURCE: { + const wchar_t *source_dest = b.sourced_file; + append_format(trace, _(L"from sourcing file %ls\n"), + user_presentable_path(source_dest).c_str()); + break; + } + case FUNCTION_CALL: + case FUNCTION_CALL_NO_SHADOW: { + append_format(trace, _(L"in function '%ls'"), b.function_name.c_str()); + // Print arguments on the same line. + wcstring args_str; + for (const wcstring &arg : b.function_args) { + if (!args_str.empty()) args_str.push_back(L' '); + // We can't quote the arguments because we print this in quotes. + // As a special-case, add the empty argument as "". + if (!arg.empty()) { + args_str.append(escape_string(arg, ESCAPE_ALL | ESCAPE_NO_QUOTED)); + } else { + args_str.append(L"\"\""); + } } + if (!args_str.empty()) { + // TODO: Escape these. + append_format(trace, _(L" with arguments '%ls'"), args_str.c_str()); + } + trace.push_back('\n'); + break; } - if (!args_str.empty()) { - // TODO: Escape these. - append_format(*buff, _(L" with arguments '%ls'"), args_str.c_str()); + case SUBST: { + append_format(trace, _(L"in command substitution\n")); + break; + } + default: { + break; // can't get here } - buff->push_back('\n'); - break; } - case SUBST: { - append_format(*buff, _(L"in command substitution\n")); - break; - } - default: { - break; // can't get here - } - } - // Print where the function is called. - const wchar_t *file = b->src_filename; + // Print where the function is called. + const wchar_t *file = b.src_filename; - if (file) { - append_format(*buff, _(L"\tcalled on line %d of file %ls\n"), b->src_lineno, - user_presentable_path(file).c_str()); - } else if (is_within_fish_initialization()) { - append_format(*buff, _(L"\tcalled during startup\n")); - } else { - // This one is way too noisy - // append_format(*buff, _(L"\tcalled on standard input\n")); + if (file) { + append_format(trace, _(L"\tcalled on line %d of file %ls\n"), b.src_lineno, + user_presentable_path(file).c_str()); + } else if (is_within_fish_initialization()) { + append_format(trace, _(L"\tcalled during startup\n")); + } else { + // This one is way too noisy + // append_format(*buff, _(L"\tcalled on standard input\n")); + } } } - // Recursively print the next block. - parser_t::stack_trace_internal(block_idx + 1, buff); + return trace; } /// Returns the name of the currently evaluated function if we are currently evaluating a function, @@ -434,18 +420,17 @@ const wchar_t *parser_t::is_function(size_t idx) const { // PCA: Have to make this a string somehow. ASSERT_IS_MAIN_THREAD(); - const wchar_t *result = nullptr; - for (size_t block_idx = idx; block_idx < this->block_count(); block_idx++) { - const block_t *b = this->block_at_index(block_idx); - if (b->type() == FUNCTION_CALL || b->type() == FUNCTION_CALL_NO_SHADOW) { - result = b->function_name.c_str(); - break; - } else if (b->type() == SOURCE) { - // If a function sources a file, obviously that function's offset doesn't contribute. + for (size_t block_idx = idx; block_idx < block_list.size(); block_idx++) { + const block_t &b = block_list[block_idx]; + if (b.is_function()) { + return b.function_name.c_str(); + } else if (b.type() == SOURCE) { + // If a function sources a file, obviously that function's offset doesn't + // contribute. break; } } - return result; + return nullptr; } /// Return the function name for the specified stack frame. Default is zero (current frame). @@ -454,13 +439,14 @@ const wchar_t *parser_t::get_function_name(int level) { if (level == 0) { // Return the function name for the level preceding the most recent breakpoint. If there // isn't one return the function name for the current level. - int idx = 0; - for (const auto &b : block_stack) { - const enum block_type_t type = b.type(); - if (type == BREAKPOINT) { - return this->is_function(idx); + // Walk until we find a breakpoint, then take the next function. + bool found_breakpoint = false; + for (const auto &b : block_list) { + if (b.type() == BREAKPOINT) { + found_breakpoint = true; + } else if (found_breakpoint && b.is_function()) { + return b.function_name.c_str(); } - idx++; } return nullptr; // couldn't find a breakpoint frame } else if (level == 1) { @@ -468,14 +454,15 @@ const wchar_t *parser_t::get_function_name(int level) { return this->is_function(); } - // Return the function name for the specific function stack frame. - int idx = 0; - for (const auto &b : block_stack) { - const enum block_type_t type = b.type(); - if (type == FUNCTION_CALL || type == FUNCTION_CALL_NO_SHADOW) { - if (--level == 0) return this->is_function(idx); + // Level 1 is the topmost function call. Level 2 is its caller. Etc. + int funcs_seen = 0; + for (const auto &b : block_list) { + if (b.is_function()) { + funcs_seen++; + if (funcs_seen == level) { + return b.function_name.c_str(); + } } - idx++; } return nullptr; // couldn't find that function level } @@ -491,15 +478,13 @@ int parser_t::get_lineno() const { const wchar_t *parser_t::current_filename() const { ASSERT_IS_MAIN_THREAD(); - for (size_t i = 0; i < this->block_count(); i++) { - const block_t *b = this->block_at_index(i); - if (b->type() == FUNCTION_CALL || b->type() == FUNCTION_CALL_NO_SHADOW) { - return function_get_definition_file(b->function_name); - } else if (b->type() == SOURCE) { - return b->sourced_file; + for (const auto &b : block_list) { + if (b.is_function()) { + return function_get_definition_file(b.function_name); + } else if (b.type() == SOURCE) { + return b.sourced_file; } } - // Fall back to the file being sourced. return libdata().current_filename; } @@ -513,8 +498,8 @@ bool parser_t::function_stack_is_overflowing() const { } // Count the functions. int depth = 0; - for (const auto &b : block_stack) { - depth += (b.type() == FUNCTION_CALL || b.type() == FUNCTION_CALL_NO_SHADOW); + for (const auto &b : block_list) { + depth += b.is_function(); } return depth > FISH_MAX_STACK_DEPTH; } @@ -656,7 +641,7 @@ eval_result_t parser_t::eval_node(parsed_source_ref_t ps, tnode_t node, block // successfully cancel (or there was nothing to cancel); clear the flag. If our block stack is // not empty, we are still in the process of cancelling; refuse to evaluate anything. if (this->cancellation_requested) { - if (!block_stack.empty()) { + if (!block_list.empty()) { return eval_result_t::cancelled; } this->cancellation_requested = false; diff --git a/src/parser.h b/src/parser.h index 41aff1176..41fdb8999 100644 --- a/src/parser.h +++ b/src/parser.h @@ -90,6 +90,11 @@ class block_t { /// Description of the block, for debugging. wcstring description() const; + /// \return if we are a function call (with or without shadowing). + bool is_function() const { + return type() == FUNCTION_CALL || type() == FUNCTION_CALL_NO_SHADOW; + } + /// Entry points for creating blocks. static block_t if_block(); static block_t event_block(event_t evt); @@ -203,7 +208,9 @@ class parser_t : public std::enable_shared_from_this { job_list_t job_list; /// The list of blocks. This is a deque because we give out raw pointers to callers, who hold /// them across manipulating this stack. - std::deque block_stack; + /// This is in "reverse" order: the topmost block is at the front. This enables iteration from + /// top down using range-based for loops. + std::deque block_list; /// The 'depth' of the fish call stack. int eval_level = -1; /// Set of variables for the parser. @@ -232,9 +239,6 @@ class parser_t : public std::enable_shared_from_this { // Given a file path, return something nicer. Currently we just "unexpand" tildes. wcstring user_presentable_path(const wcstring &path) const; - /// Helper for stack_trace(). - void stack_trace_internal(size_t block_idx, wcstring *buff) const; - /// Create a parser. parser_t(); parser_t(std::shared_ptr vars); @@ -289,17 +293,17 @@ class parser_t : public std::enable_shared_from_this { /// Returns the current line number. int get_lineno() const; - /// Returns the block at the given index. 0 corresponds to the innermost block. Returns NULL + /// Returns the block at the given index. 0 corresponds to the innermost block. Returns nullptr /// when idx is at or equal to the number of blocks. const block_t *block_at_index(size_t idx) const; block_t *block_at_index(size_t idx); + /// Return the list of blocks. The first block is at the top. + const std::deque &blocks() const { return block_list; } + /// Returns the current (innermost) block. block_t *current_block(); - /// Count of blocks. - size_t block_count() const { return block_stack.size(); } - /// Get the list of jobs. job_list_t &jobs() { return job_list; } const job_list_t &jobs() const { return job_list; } diff --git a/src/reader.cpp b/src/reader.cpp index 15e915f57..517c94d13 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -2204,8 +2204,8 @@ void reader_bg_job_warning(const parser_t &parser) { /// interactive mode. It checks if it is ok to exit. static void handle_end_loop(const parser_t &parser) { if (!reader_exit_forced()) { - for (size_t i = 0; i < parser.block_count(); i++) { - if (parser.block_at_index(i)->type() == BREAKPOINT) { + for (const auto &b : parser.blocks()) { + if (b.type() == BREAKPOINT) { // We're executing within a breakpoint so we do not want to terminate the shell and // kill background jobs. return; diff --git a/src/trace.cpp b/src/trace.cpp index d7968c9c4..f247ac5f8 100644 --- a/src/trace.cpp +++ b/src/trace.cpp @@ -19,7 +19,7 @@ bool trace_enabled(const parser_t &parser) { void trace_argv(const parser_t &parser, const wchar_t *command, const wcstring_list_t &argv) { // Format into a string to prevent interleaving with flog in other threads. // Add the + prefix. - wcstring trace_text(parser.block_count(), '+'); + wcstring trace_text(parser.blocks().size(), '+'); if (command && command[0]) { trace_text.push_back(L' ');