From 73eea8e47ac1db5d4688ff100df3ec6b51c76e71 Mon Sep 17 00:00:00 2001
From: Jon Meow <46229924+jonmeow@users.noreply.github.com>
Date: Mon, 8 Nov 2021 09:22:38 -0800
Subject: [PATCH] Merge UnwindTo and UnwindPast logic (#926)

Also shifts the DeallocateScope concept into UnwindTodoTop since that's how it's used.
---
 .../interpreter/interpreter.cpp               | 72 +++++++++----------
 .../interpreter/interpreter.h                 |  2 +-
 2 files changed, 34 insertions(+), 40 deletions(-)

diff --git a/executable_semantics/interpreter/interpreter.cpp b/executable_semantics/interpreter/interpreter.cpp
index 9ba4f21cc1ff..981a64d34885 100644
--- a/executable_semantics/interpreter/interpreter.cpp
+++ b/executable_semantics/interpreter/interpreter.cpp
@@ -181,14 +181,18 @@ void Interpreter::InitGlobals(llvm::ArrayRef<Nonnull<Declaration*>> fs) {
   }
 }
 
-void Interpreter::DeallocateScope(Scope& scope) {
-  CHECK(!scope.deallocated);
-  for (const auto& l : scope.locals) {
-    std::optional<AllocationId> a = scope.values.Get(l);
-    CHECK(a);
-    heap_.Deallocate(*a);
+auto Interpreter::UnwindTodoTop() -> Nonnull<Action*> {
+  Nonnull<Action*> act = todo_.Pop();
+  if (act->scope().has_value()) {
+    CHECK(!act->scope()->deallocated);
+    for (const auto& l : act->scope()->locals) {
+      std::optional<AllocationId> a = act->scope()->values.Get(l);
+      CHECK(a);
+      heap_.Deallocate(*a);
+    }
+    act->scope()->deallocated = true;
   }
-  scope.deallocated = true;
+  return act;
 }
 
 auto Interpreter::CreateTuple(Nonnull<Action*> act,
@@ -848,13 +852,13 @@ auto Interpreter::StepStmt() -> Transition {
       CHECK(act->pos() == 0);
       //    { { break; :: ... :: (while (e) s) :: C, E, F} :: S, H}
       // -> { { C, E', F} :: S, H}
-      return UnwindPast{&cast<Break>(stmt).loop()};
+      return UnwindPast{.ast_node = &cast<Break>(stmt).loop()};
     }
     case Statement::Kind::Continue: {
       CHECK(act->pos() == 0);
       //    { { continue; :: ... :: (while (e) s) :: C, E, F} :: S, H}
       // -> { { (while (e) s) :: C, E', F} :: S, H}
-      return UnwindTo{&cast<Continue>(stmt).loop()};
+      return UnwindTo{.ast_node = &cast<Continue>(stmt).loop()};
     }
     case Statement::Kind::Block: {
       const auto& block = cast<Block>(stmt);
@@ -963,7 +967,8 @@ auto Interpreter::StepStmt() -> Transition {
         // TODO(geoffromer): convert the result to the function's return type,
         // once #880 gives us a way to find that type.
         const FunctionDeclaration& function = cast<Return>(stmt).function();
-        return UnwindPast{*function.body(), act->results()[0]};
+        return UnwindPast{.ast_node = *function.body(),
+                          .result = act->results()[0]};
       }
     case Statement::Kind::Continuation: {
       CHECK(act->pos() == 0);
@@ -1024,10 +1029,7 @@ class Interpreter::DoTransition {
   explicit DoTransition(Interpreter* interpreter) : interpreter(interpreter) {}
 
   void operator()(const Done& done) {
-    Nonnull<Action*> act = interpreter->todo_.Pop();
-    if (act->scope().has_value()) {
-      interpreter->DeallocateScope(*act->scope());
-    }
+    Nonnull<Action*> act = interpreter->UnwindTodoTop();
     switch (act->kind()) {
       case Action::Kind::ExpressionAction:
       case Action::Kind::LValAction:
@@ -1065,33 +1067,12 @@ class Interpreter::DoTransition {
     action->set_pos(action->pos() + 1);
   }
 
-  void operator()(const UnwindTo& unwind_to) {
-    while (true) {
-      if (const auto* statement_action =
-              dyn_cast<StatementAction>(interpreter->todo_.Top());
-          statement_action != nullptr &&
-          &statement_action->statement() == unwind_to.ast_node) {
-        break;
-      }
-      Nonnull<Action*> action = interpreter->todo_.Pop();
-      if (action->scope().has_value()) {
-        interpreter->DeallocateScope(*action->scope());
-      }
-    }
-  }
+  void operator()(const UnwindTo& unwind_to) { DoUnwindTo(unwind_to.ast_node); }
 
   void operator()(const UnwindPast& unwind_past) {
-    while (true) {
-      Nonnull<Action*> action = interpreter->todo_.Pop();
-      if (action->scope().has_value()) {
-        interpreter->DeallocateScope(*action->scope());
-      }
-      if (const auto* statement_action = dyn_cast<StatementAction>(action);
-          statement_action != nullptr &&
-          &statement_action->statement() == unwind_past.ast_node) {
-        break;
-      }
-    }
+    DoUnwindTo(unwind_past.ast_node);
+    // Unwind past the statement and return a result if needed.
+    interpreter->UnwindTodoTop();
     if (unwind_past.result.has_value()) {
       interpreter->todo_.Top()->AddResult(*unwind_past.result);
     }
@@ -1123,6 +1104,19 @@ class Interpreter::DoTransition {
   void operator()(const ManualTransition&) {}
 
  private:
+  // Unwinds to the indicated node.
+  void DoUnwindTo(Nonnull<const Statement*> ast_node) {
+    while (true) {
+      if (const auto* statement_action =
+              dyn_cast<StatementAction>(interpreter->todo_.Top());
+          statement_action != nullptr &&
+          &statement_action->statement() == ast_node) {
+        break;
+      }
+      interpreter->UnwindTodoTop();
+    }
+  }
+
   Nonnull<Interpreter*> interpreter;
 };
 
diff --git a/executable_semantics/interpreter/interpreter.h b/executable_semantics/interpreter/interpreter.h
index 8f763bf178e4..23614ff9802b 100644
--- a/executable_semantics/interpreter/interpreter.h
+++ b/executable_semantics/interpreter/interpreter.h
@@ -137,7 +137,7 @@ class Interpreter {
   auto GetFromEnv(SourceLocation source_loc, const std::string& name)
       -> Address;
 
-  void DeallocateScope(Scope& scope);
+  auto UnwindTodoTop() -> Nonnull<Action*>;
 
   auto CreateTuple(Nonnull<Action*> act, Nonnull<const Expression*> exp)
       -> Nonnull<const Value*>;