From b954542d064c36a8237a5f4e30cbea1afb981526 Mon Sep 17 00:00:00 2001
From: Geoff Romer <gromer@google.com>
Date: Tue, 18 Jan 2022 13:29:44 -0800
Subject: [PATCH] Reorganize and document Action (#1024)

---
 executable_semantics/interpreter/action.cpp |  8 ---
 executable_semantics/interpreter/action.h   | 68 +++++++++++----------
 2 files changed, 37 insertions(+), 39 deletions(-)

diff --git a/executable_semantics/interpreter/action.cpp b/executable_semantics/interpreter/action.cpp
index 9d7e8f9624ad..ed5129137414 100644
--- a/executable_semantics/interpreter/action.cpp
+++ b/executable_semantics/interpreter/action.cpp
@@ -72,12 +72,4 @@ void Action::Print(llvm::raw_ostream& out) const {
   }
 }
 
-void Action::PrintList(const Stack<Nonnull<Action*>>& ls,
-                       llvm::raw_ostream& out) {
-  llvm::ListSeparator sep(" :: ");
-  for (const auto& action : ls) {
-    out << sep << *action;
-  }
-}
-
 }  // namespace Carbon
diff --git a/executable_semantics/interpreter/action.h b/executable_semantics/interpreter/action.h
index 5c99e83df0c4..30b4239f398f 100644
--- a/executable_semantics/interpreter/action.h
+++ b/executable_semantics/interpreter/action.h
@@ -58,6 +58,18 @@ class Scope {
   Nonnull<HeapAllocationInterface*> heap_;
 };
 
+// An Action represents the current state of a self-contained computation,
+// usually associated with some AST node, such as evaluation of an expression or
+// execution of a statement. Execution of an action is divided into a series of
+// steps, and the `pos` field typically counts the number of steps executed.
+//
+// They should be destroyed as soon as they are done executing, in order to
+// clean up the associated Carbon scope, and consequently they should not be
+// allocated on an Arena. Actions are typically owned by the ActionStack.
+//
+// The actual behavior of an Action step is defined by Interpreter::Step, not by
+// Action or its subclasses.
+// TODO: consider moving this logic to a virtual method `Step`.
 class Action {
  public:
   enum class Kind {
@@ -72,14 +84,38 @@ class Action {
   Action(const Value&) = delete;
   auto operator=(const Value&) -> Action& = delete;
 
-  void AddResult(Nonnull<const Value*> result) { results_.push_back(result); }
+  virtual ~Action() = default;
 
+  void Print(llvm::raw_ostream& out) const;
+  LLVM_DUMP_METHOD void Dump() const { Print(llvm::errs()); }
+
+  // Resets this Action to its initial state.
   void Clear() {
     CHECK(!scope_.has_value());
     pos_ = 0;
     results_.clear();
   }
 
+  // Returns the enumerator corresponding to the most-derived type of this
+  // object.
+  auto kind() const -> Kind { return kind_; }
+
+  // The position or state of the action. Starts at 0 and is typically
+  // incremented after each step.
+  auto pos() const -> int { return pos_; }
+  void set_pos(int pos) { this->pos_ = pos; }
+
+  // The results of any Actions spawned by this Action.
+  auto results() const -> const std::vector<Nonnull<const Value*>>& {
+    return results_;
+  }
+
+  // Appends `result` to `results`.
+  void AddResult(Nonnull<const Value*> result) { results_.push_back(result); }
+
+  // Returns the scope associated with this Action, if any.
+  auto scope() -> std::optional<Scope>& { return scope_; }
+
   // Associates this action with a new scope, with initial state `scope`.
   // Values that are local to this scope will be deallocated when this
   // Action is completed or unwound. Can only be called once on a given
@@ -89,36 +125,6 @@ class Action {
     scope_ = std::move(scope);
   }
 
-  // Returns the scope associated with this Action, if any.
-  auto scope() -> std::optional<Scope>& { return scope_; }
-
-  static void PrintList(const Stack<Nonnull<Action*>>& ls,
-                        llvm::raw_ostream& out);
-
-  void Print(llvm::raw_ostream& out) const;
-  LLVM_DUMP_METHOD void Dump() const { Print(llvm::errs()); }
-
-  // Returns the enumerator corresponding to the most-derived type of this
-  // object.
-  auto kind() const -> Kind { return kind_; }
-
-  // The position or state of the action. Starts at 0 and goes up to the number
-  // of subexpressions.
-  //
-  // pos indicates how many of the entries in the following `results` vector
-  // will be filled in the next time this action is active.
-  // For each i < pos, results[i] contains a pointer to a Value.
-  auto pos() const -> int { return pos_; }
-
-  void set_pos(int pos) { this->pos_ = pos; }
-
-  // Results from a subexpression.
-  auto results() const -> const std::vector<Nonnull<const Value*>>& {
-    return results_;
-  }
-
-  virtual ~Action() = default;
-
  protected:
   // Constructs an Action. `kind` must be the enumerator corresponding to the
   // most-derived type being constructed.