From 0991e398bb7eb898d9b23ef57413d2185e5522cb Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 21 Jan 2017 15:35:35 -0800 Subject: [PATCH] Clean up parser_t's block stack Currently the block stack is just a vector of pointers. Clients must manually use new() to allocate a block, and then transfer ownership to the stack (so must NOT delete it). Give the parser itself responsibility for allocating blocks too, so that it takes over both allocation and deletion. Use unique_ptr to make deletion less error-prone. --- src/builtin.cpp | 8 ++++---- src/event.cpp | 5 ++--- src/exec.cpp | 6 +++--- src/parse_execution.cpp | 24 +++++++----------------- src/parser.cpp | 38 +++++++++++++++++--------------------- src/parser.h | 21 ++++++++++++++------- 6 files changed, 47 insertions(+), 55 deletions(-) diff --git a/src/builtin.cpp b/src/builtin.cpp index 9c6f33609..59a97f60b 100644 --- a/src/builtin.cpp +++ b/src/builtin.cpp @@ -2838,14 +2838,14 @@ static int builtin_source(parser_t &parser, io_streams_t &streams, wchar_t **arg fn_intern = intern(argv[1]); } - parser.push_block(new source_block_t(fn_intern)); + const source_block_t *sb = parser.push_block(fn_intern); reader_push_current_filename(fn_intern); env_set_argv(argc > 1 ? argv + 2 : argv + 1); res = reader_read(fd, streams.io_chain ? *streams.io_chain : io_chain_t()); - parser.pop_block(); + parser.pop_block(sb); if (res) { streams.err.append_format(_(L"%ls: Error while reading file '%ls'\n"), argv[0], @@ -3052,11 +3052,11 @@ static int builtin_breakpoint(parser_t &parser, io_streams_t &streams, wchar_t * return STATUS_BUILTIN_ERROR; } - parser.push_block(new breakpoint_block_t()); + const breakpoint_block_t *bpb = parser.push_block(); reader_read(STDIN_FILENO, streams.io_chain ? *streams.io_chain : io_chain_t()); - parser.pop_block(); + parser.pop_block(bpb); return proc_get_last_status(); } diff --git a/src/event.cpp b/src/event.cpp index e365f5dfa..ed72f283c 100644 --- a/src/event.cpp +++ b/src/event.cpp @@ -394,10 +394,9 @@ static void event_fire_internal(const event_t &event) { prev_status = proc_get_last_status(); parser_t &parser = parser_t::principal_parser(); - block_t *block = new event_block_t(event); - parser.push_block(block); + event_block_t *b = parser.push_block(event); parser.eval(buffer, io_chain_t(), TOP); - parser.pop_block(); + parser.pop_block(b); proc_pop_interactive(); proc_set_last_status(prev_status); } diff --git a/src/exec.cpp b/src/exec.cpp index ca98afa50..33cce87ad 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -629,8 +629,8 @@ void exec_job(parser_t &parser, job_t *j) { debug(0, _(L"Unknown function '%ls'"), p->argv0()); break; } - function_block_t *newv = new function_block_t(p, func_name, shadow_scope); - parser.push_block(newv); + + function_block_t *fb = parser.push_block(p, func_name, shadow_scope); // Setting variables might trigger an event handler, hence we need to unblock // signals. @@ -661,7 +661,7 @@ void exec_job(parser_t &parser, job_t *j) { } parser.allow_function(); - parser.pop_block(); + parser.pop_block(fb); break; } diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 72e0b72df..1073bd44b 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -266,9 +266,8 @@ parse_execution_result_t parse_execution_context_t::run_if_statement( assert(statement.type == symbol_if_statement); // Push an if block. - if_block_t *ib = new if_block_t(); + if_block_t *ib = parser->push_block(); ib->node_offset = this->get_offset(statement); - parser->push_block(ib); parse_execution_result_t result = parse_execution_success; @@ -348,14 +347,9 @@ parse_execution_result_t parse_execution_context_t::run_begin_statement( assert(header.type == symbol_begin_header); assert(contents.type == symbol_job_list); - // Basic begin/end block. Push a scope block. - scope_block_t *sb = new scope_block_t(BEGIN); - parser->push_block(sb); - - // Run the job list. + // Basic begin/end block. Push a scope block, run jobs, pop it + scope_block_t *sb = parser->push_block(BEGIN); parse_execution_result_t ret = run_job_list(contents, sb); - - // Pop the block. parser->pop_block(sb); return ret; @@ -470,8 +464,7 @@ parse_execution_result_t parse_execution_context_t::run_for_statement( return ret; } - for_block_t *fb = new for_block_t(); - parser->push_block(fb); + for_block_t *fb = parser->push_block(); // Now drive the for loop. const size_t arg_count = argument_sequence.size(); @@ -552,8 +545,7 @@ parse_execution_result_t parse_execution_context_t::run_switch_statement( const wcstring &switch_value_expanded = switch_values_expanded.at(0).completion; - switch_block_t *sb = new switch_block_t(); - parser->push_block(sb); + switch_block_t *sb = parser->push_block(); // Expand case statements. const parse_node_t *case_item_list = get_child(statement, 3, symbol_case_item_list); @@ -616,9 +608,8 @@ parse_execution_result_t parse_execution_context_t::run_while_statement( assert(block_contents.type == symbol_job_list); // Push a while block. - while_block_t *wb = new while_block_t(); + while_block_t *wb = parser->push_block(); wb->node_offset = this->get_offset(header); - parser->push_block(wb); parse_execution_result_t ret = parse_execution_success; @@ -666,9 +657,8 @@ parse_execution_result_t parse_execution_context_t::run_while_statement( } } - /* Done */ + // Done parser->pop_block(wb); - return ret; } diff --git a/src/parser.cpp b/src/parser.cpp index bfc14eaed..7d9901cdc 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -122,7 +122,8 @@ void parser_t::skip_all_blocks(void) { s_principal_parser.cancellation_requested = true; } -void parser_t::push_block(block_t *new_current) { +// Given a new-allocated block, push it onto our block stack, acquiring ownership +void parser_t::push_block_int(block_t *new_current) { const enum block_type_t type = new_current->type(); new_current->src_lineno = parser_t::get_lineno(); @@ -144,7 +145,8 @@ void parser_t::push_block(block_t *new_current) { new_current->job = nullptr; new_current->loop_status = LOOP_NORMAL; - this->block_stack.push_back(new_current); + // Push it onto our stack. This acquires ownership because of unique_ptr. + this->block_stack.emplace_back(new_current); // Types TOP and SUBST are not considered blocks for the purposes of `status -b`. if (type != TOP && type != SUBST) { @@ -157,25 +159,25 @@ void parser_t::push_block(block_t *new_current) { } } -void parser_t::pop_block() { +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; } - block_t *old = block_stack.back(); + // acquire ownership out of the block stack + // this will trigger deletion when it goes out of scope + std::unique_ptr old = std::move(block_stack.back()); block_stack.pop_back(); if (old->wants_pop_env) env_pop(); - delete old; - // Figure out if `status -b` should consider us to be in a block now. int new_is_block = 0; - for (std::vector::const_iterator it = block_stack.begin(), end = block_stack.end(); - it != end; ++it) { - const enum block_type_t type = (*it)->type(); + for (const auto &b : block_stack) { + const enum block_type_t type = b->type(); if (type != TOP && type != SUBST) { new_is_block = 1; break; @@ -184,11 +186,6 @@ void parser_t::pop_block() { is_block = new_is_block; } -void parser_t::pop_block(const block_t *expected) { - assert(expected == this->current_block()); - this->pop_block(); -} - const wchar_t *parser_t::get_block_desc(int block) const { for (size_t i = 0; block_lookup[i].desc; i++) { if (block_lookup[i].type == block) { @@ -221,15 +218,15 @@ wcstring parser_t::block_stack_description() const { 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) : NULL; + return idx < count ? block_stack.at(count - idx - 1).get() : NULL; } 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) : NULL; + return idx < count ? block_stack.at(count - idx - 1).get() : NULL; } -block_t *parser_t::current_block() { return block_stack.empty() ? NULL : block_stack.back(); } +block_t *parser_t::current_block() { return block_stack.empty() ? NULL : block_stack.back().get(); } void parser_t::forbid_function(const wcstring &function) { forbidden_function.push_back(function); } @@ -657,12 +654,11 @@ int parser_t::eval_block_node(node_offset_t node_idx, const io_chain_t &io, // Start it up const block_t *const start_current_block = current_block(); - block_t *scope_block = new scope_block_t(block_type); - this->push_block(scope_block); + scope_block_t *scope_block = this->push_block(block_type); int result = ctx->eval_node_at_offset(node_idx, scope_block, io); // Clean up the block stack. - this->pop_block(); + this->pop_block(scope_block); while (start_current_block != current_block()) { if (current_block() == NULL) { debug(0, _(L"End of block mismatch. Program terminating.")); @@ -670,7 +666,7 @@ int parser_t::eval_block_node(node_offset_t node_idx, const io_chain_t &io, FATAL_EXIT(); break; } - this->pop_block(); + this->pop_block(current_block()); } job_reap(0); // reap again diff --git a/src/parser.h b/src/parser.h index 586c3fc3f..1e2841262 100644 --- a/src/parser.h +++ b/src/parser.h @@ -187,8 +187,8 @@ class parser_t { wcstring_list_t forbidden_function; /// The jobs associated with this parser. job_list_t my_job_list; - /// The list of blocks, allocated with new. It's our responsibility to delete these. - std::vector block_stack; + /// The list of blocks + std::vector> block_stack; #if 0 // TODO: Lint says this isn't used (which is true). Should this be removed? @@ -217,6 +217,9 @@ class parser_t { /// Helper for stack_trace(). void stack_trace_internal(size_t block_idx, wcstring *out) const; + /// Helper for push_block() + void push_block_int(block_t *b); + public: /// Get the "principal" parser, whatever that is. static parser_t &principal_parser(); @@ -284,11 +287,15 @@ class parser_t { // know whether it's run during initialization or not. void set_is_within_fish_initialization(bool flag); - /// Pushes the block. pop_block will call delete on it. - void push_block(block_t *newv); - - /// Remove the outermost block namespace. - void pop_block(); + /// Pushes a new block created with the given arguments + /// Returns a pointer to the block. The pointer is valid + /// until the call to pop_block() + template + BLOCKTYPE *push_block(Args&&... args) { + BLOCKTYPE *ret = new BLOCKTYPE(std::forward(args)...); + this->push_block_int(ret); + return ret; + } /// Remove the outermost block, asserting it's the given one. void pop_block(const block_t *b);