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

@@ -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<source_block_t>(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<breakpoint_block_t>();
 
     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();
 }

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);
+        event_block_t *b = parser.push_block<event_block_t>(event);
-        parser.push_block(block);
         parser.eval(buffer, io_chain_t(), TOP);
-        parser.pop_block();
+        parser.pop_block(b);
         proc_pop_interactive();
         proc_set_last_status(prev_status);
     }

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<function_block_t>(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;
             }

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<if_block_t>();
     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.
+    // Basic begin/end block. Push a scope block, run jobs, pop it
-    scope_block_t *sb = new scope_block_t(BEGIN);
+    scope_block_t *sb = parser->push_block<scope_block_t>(BEGIN);
-    parser->push_block(sb);
-
-    // Run the job list.
     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();
+    for_block_t *fb = parser->push_block<for_block_t>();
-    parser->push_block(fb);
 
     // 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();
+    switch_block_t *sb = parser->push_block<switch_block_t>();
-    parser->push_block(sb);
 
     // 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<while_block_t>();
     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;
 }
 

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<block_t> 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<block_t *>::const_iterator it = block_stack.begin(), end = block_stack.end();
+    for (const auto &b : block_stack) {
-         it != end; ++it) {
+        const enum block_type_t type = b->type();
-        const enum block_type_t type = (*it)->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);
+    scope_block_t *scope_block = this->push_block<scope_block_t>(block_type);
-    this->push_block(scope_block);
     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

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.
+    /// The list of blocks
-    std::vector<block_t *> block_stack;
+    std::vector<std::unique_ptr<block_t>> 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.
+    /// Pushes a new block created with the given arguments
-    void push_block(block_t *newv);
+    /// Returns a pointer to the block. The pointer is valid
-
+    /// until the call to pop_block()
-    /// Remove the outermost block namespace.
+    template<typename BLOCKTYPE, typename... Args>
-    void pop_block();
+    BLOCKTYPE *push_block(Args&&... args) {
+        BLOCKTYPE *ret = new BLOCKTYPE(std::forward<Args>(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);