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);