From 5c74a6147b4b81dc5b173f190f02f6681ec4b0fe Mon Sep 17 00:00:00 2001
From: John Ericson <John.Ericson@Obsidian.Systems>
Date: Sun, 11 Oct 2020 17:07:14 +0000
Subject: [PATCH] Properly type the derivation and substitution goal maps

As a bonus, Worker::removeGoal is less inefficient.
---
 src/libstore/build.hh        | 11 +++++------
 src/libstore/build/worker.cc | 33 +++++++++++++++++----------------
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/src/libstore/build.hh b/src/libstore/build.hh
index 8027c61b1..894feab5b 100644
--- a/src/libstore/build.hh
+++ b/src/libstore/build.hh
@@ -28,7 +28,6 @@ struct HookInstance;
 
 /* A pointer to a goal. */
 struct Goal;
-class DerivationGoal;
 typedef std::shared_ptr<Goal> GoalPtr;
 typedef std::weak_ptr<Goal> WeakGoalPtr;
 
@@ -140,6 +139,8 @@ struct Child
     steady_time_point timeStarted;
 };
 
+class DerivationGoal;
+class SubstitutionGoal;
 
 /* The worker class. */
 class Worker
@@ -167,8 +168,8 @@ private:
 
     /* Maps used to prevent multiple instantiations of a goal for the
        same derivation / path. */
-    WeakGoalMap derivationGoals;
-    WeakGoalMap substitutionGoals;
+    std::map<StorePath, std::weak_ptr<DerivationGoal>> derivationGoals;
+    std::map<StorePath, std::weak_ptr<SubstitutionGoal>> substitutionGoals;
 
     /* Goals waiting for busy paths to be unlocked. */
     WeakGoals waitingForAnyGoal;
@@ -242,7 +243,7 @@ public:
         const StringSet & wantedOutputs, BuildMode buildMode = bmNormal);
 
     /* substitution goal */
-    GoalPtr makeSubstitutionGoal(const StorePath & storePath, RepairFlag repair = NoRepair, std::optional<ContentAddress> ca = std::nullopt);
+    std::shared_ptr<SubstitutionGoal> makeSubstitutionGoal(const StorePath & storePath, RepairFlag repair = NoRepair, std::optional<ContentAddress> ca = std::nullopt);
 
     /* Remove a dead goal. */
     void removeGoal(GoalPtr goal);
@@ -305,8 +306,6 @@ public:
 
 typedef enum {rpAccept, rpDecline, rpPostpone} HookReply;
 
-class SubstitutionGoal;
-
 /* Unless we are repairing, we don't both to test validity and just assume it,
    so the choices are `Absent` or `Valid`. */
 enum struct PathStatus {
diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc
index 2fc9f6982..47403580e 100644
--- a/src/libstore/build/worker.cc
+++ b/src/libstore/build/worker.cc
@@ -39,16 +39,13 @@ std::shared_ptr<DerivationGoal> Worker::makeDerivationGoalCommon(
     const StringSet & wantedOutputs,
     std::function<std::shared_ptr<DerivationGoal>()> mkDrvGoal)
 {
-    WeakGoalPtr & abstract_goal_weak = derivationGoals[drvPath];
-    GoalPtr abstract_goal = abstract_goal_weak.lock(); // FIXME
-    std::shared_ptr<DerivationGoal> goal;
-    if (!abstract_goal) {
+    std::weak_ptr<DerivationGoal> & goal_weak = derivationGoals[drvPath];
+    std::shared_ptr<DerivationGoal> goal = goal_weak.lock();
+    if (!goal) {
         goal = mkDrvGoal();
-        abstract_goal_weak = goal;
+        goal_weak = goal;
         wakeUp(goal);
     } else {
-        goal = std::dynamic_pointer_cast<DerivationGoal>(abstract_goal);
-        assert(goal);
         goal->addWantedOutputs(wantedOutputs);
     }
     return goal;
@@ -73,10 +70,10 @@ std::shared_ptr<DerivationGoal> Worker::makeBasicDerivationGoal(const StorePath
 }
 
 
-GoalPtr Worker::makeSubstitutionGoal(const StorePath & path, RepairFlag repair, std::optional<ContentAddress> ca)
+std::shared_ptr<SubstitutionGoal> Worker::makeSubstitutionGoal(const StorePath & path, RepairFlag repair, std::optional<ContentAddress> ca)
 {
-    WeakGoalPtr & goal_weak = substitutionGoals[path];
-    GoalPtr goal = goal_weak.lock(); // FIXME
+    std::weak_ptr<SubstitutionGoal> & goal_weak = substitutionGoals[path];
+    auto goal = goal_weak.lock(); // FIXME
     if (!goal) {
         goal = std::make_shared<SubstitutionGoal>(path, *this, repair, ca);
         goal_weak = goal;
@@ -85,14 +82,14 @@ GoalPtr Worker::makeSubstitutionGoal(const StorePath & path, RepairFlag repair,
     return goal;
 }
 
-
-static void removeGoal(GoalPtr goal, WeakGoalMap & goalMap)
+template<typename G>
+static void removeGoal(std::shared_ptr<G> goal, std::map<StorePath, std::weak_ptr<G>> & goalMap)
 {
     /* !!! inefficient */
-    for (WeakGoalMap::iterator i = goalMap.begin();
+    for (typename std::map<StorePath, std::weak_ptr<G>>::iterator i = goalMap.begin();
          i != goalMap.end(); )
         if (i->second.lock() == goal) {
-            WeakGoalMap::iterator j = i; ++j;
+            typename std::map<StorePath, std::weak_ptr<G>>::iterator j = i; ++j;
             goalMap.erase(i);
             i = j;
         }
@@ -102,8 +99,12 @@ static void removeGoal(GoalPtr goal, WeakGoalMap & goalMap)
 
 void Worker::removeGoal(GoalPtr goal)
 {
-    nix::removeGoal(goal, derivationGoals);
-    nix::removeGoal(goal, substitutionGoals);
+    if (auto drvGoal = std::dynamic_pointer_cast<DerivationGoal>(goal))
+        nix::removeGoal(drvGoal, derivationGoals);
+    else if (auto subGoal = std::dynamic_pointer_cast<SubstitutionGoal>(goal))
+        nix::removeGoal(subGoal, substitutionGoals);
+    else
+        assert(false);
     if (topGoals.find(goal) != topGoals.end()) {
         topGoals.erase(goal);
         /* If a top-level goal failed, then kill all other goals