From 1f7de3e40a0d520917609b3417a2683c5f42ee76 Mon Sep 17 00:00:00 2001
From: Silvan Sievers <silvan.sievers@unibas.ch>
Date: Fri, 13 Oct 2023 09:24:06 +0200
Subject: [PATCH] overload lookup_state_id; document

---
 src/search/state_registry.cc | 34 ++++++++++++++++++++--------------
 src/search/state_registry.h  |  7 +++++++
 2 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/src/search/state_registry.cc b/src/search/state_registry.cc
index c8175f78c7..ff79594ca4 100644
--- a/src/search/state_registry.cc
+++ b/src/search/state_registry.cc
@@ -41,6 +41,12 @@ State StateRegistry::lookup_state(StateID id) const {
     return task_proxy.create_state(*this, id, buffer);
 }
 
+State StateRegistry::lookup_state(
+    StateID id, vector<int> &&state_values) const {
+    const PackedStateBin *buffer = state_data_pool[id.value];
+    return task_proxy.create_state(*this, id, buffer, move(state_values));
+}
+
 const State &StateRegistry::get_initial_state() {
     if (!cached_initial_state) {
         int num_bins = get_bins_per_state();
@@ -64,6 +70,11 @@ const State &StateRegistry::get_initial_state() {
 //     operating on state buffers (PackedStateBin *).
 State StateRegistry::get_successor_state(const State &predecessor, const OperatorProxy &op) {
     assert(!op.is_axiom());
+    /*
+      TODO: ideally, we would not modify state_data_pool here and in
+      insert_id_or_pop_state, but only at one place, to avoid errors like
+      buffer becoming a dangling pointer.
+    */
     state_data_pool.push_back(predecessor.get_buffer());
     PackedStateBin *buffer = state_data_pool[state_data_pool.size() - 1];
     /* Experiments for issue348 showed that for tasks with axioms it's faster
@@ -81,10 +92,12 @@ State StateRegistry::get_successor_state(const State &predecessor, const Operato
         for (size_t i = 0; i < new_values.size(); ++i) {
             state_packer.set(buffer, i, new_values[i]);
         }
+        /*
+          NOTE: insert_id_or_pop_state possibly invalidates buffer, hence
+          we use lookup_state to retrieve the state using the correct buffer.
+        */
         StateID id = insert_id_or_pop_state();
-        // See below for the case withou axioms on why we need to reset buffer.
-        buffer = state_data_pool[id.value];
-        return task_proxy.create_state(*this, id, buffer, move(new_values));
+        return lookup_state(id, move(new_values));
     } else {
         for (EffectProxy effect : op.get_effects()) {
             if (does_fire(effect, predecessor)) {
@@ -92,19 +105,12 @@ State StateRegistry::get_successor_state(const State &predecessor, const Operato
                 state_packer.set(buffer, effect_pair.var, effect_pair.value);
             }
         }
-        StateID id = insert_id_or_pop_state();
         /*
-          insert_id_or_pop_state deletes the state data pointed to by buffer
-          if the state already exists. We need to use the buffer corresponding
-          to the right state. NOTE: the below code is exactly what lookup_state
-          does, but for symmetry with the above case for axioms and for future
-          refactoring, we explicitly set buffer here.
-
-          TODO: ideally, we would not modify state_data_pool here and in
-          insert_id_or_pop_state, but only at one place.
+          NOTE: insert_id_or_pop_state possibly invalidates buffer, hence
+          we use lookup_state to retrieve the state using the correct buffer.
         */
-        buffer = state_data_pool[id.value];
-        return task_proxy.create_state(*this, id, buffer);
+        StateID id = insert_id_or_pop_state();
+        return lookup_state(id);
     }
 }
 
diff --git a/src/search/state_registry.h b/src/search/state_registry.h
index bb35af642b..0604b0fff5 100644
--- a/src/search/state_registry.h
+++ b/src/search/state_registry.h
@@ -190,6 +190,13 @@ class StateRegistry : public subscriber::SubscriberService<StateRegistry> {
     */
     State lookup_state(StateID id) const;
 
+    /*
+      Like lookup_state above, but creates a state with unpacked data,
+      moved in via state_values. It is the caller's responsibility that
+      the unpacked data matches the state's data.
+    */
+    State lookup_state(StateID id, std::vector<int> &&state_values) const;
+
     /*
       Returns a reference to the initial state and registers it if this was not
       done before. The result is cached internally so subsequent calls are cheap.