Rework postponed logic

Specifically, now postponed is only used by the execution mode logic
and rules should not return it directly.

12 files changed, 88 insertions, 75 deletions

diff --git a/build/algorithm b/build/algorithm
index 99e6c2b..af461dc 100644
--- a/build/algorithm
+++ b/build/algorithm
@@ -67,13 +67,15 @@ namespace build
   // Match and apply a rule to the action/target with ambiguity
   // detection. Increment the target's dependents count, which
   // means that you should call this function with the intent
-  // to also call execute(). In case of optimizations that
-  // would avoid calling execute(), decrement the dependents
-  // cound manually to compensate.
+  // to also call execute(). In case of optimizations that would
+  // avoid calling execute(), call unmatch() to indicate this.
   //
   void
   match (action, target&);
 
+  void
+  unmatch (action, target&);
+
   // Match (but do not apply) a rule to the action/target with
   // ambiguity detection. Note that this function does not touch
   // the dependents count.
@@ -84,7 +86,7 @@ namespace build
   // Match a "delegate rule" from withing another rules' apply()
   // function. Return recipe and recipe action (if any). Note
   // that unlike match(), this call doesn't increment the
-  // dependents count. See also execute_delegate().
+  // dependents count. See also the companion execute_delegate().
   //
   std::pair<recipe, action>
   match_delegate (action, target&);
diff --git a/build/algorithm.cxx b/build/algorithm.cxx
index 94c84b8..e09d125 100644
--- a/build/algorithm.cxx
+++ b/build/algorithm.cxx
@@ -338,7 +338,7 @@ namespace build
     //
     switch (t.raw_state)
     {
-    case target_state::group:
+    case target_state::group: // Means group's state is unknown.
     case target_state::unknown:
     case target_state::postponed:
       {
diff --git a/build/algorithm.ixx b/build/algorithm.ixx
index bb66b53..e0b9364 100644
--- a/build/algorithm.ixx
+++ b/build/algorithm.ixx
@@ -60,6 +60,19 @@ namespace build
       match_impl (a, t, true);
 
     t.dependents++;
+    dependency_count++;
+
+    // text << "M " << t << ": " << t.dependents << " " << dependency_count;
+  }
+
+  inline void
+  unmatch (action, target& t)
+  {
+    assert (t.dependents != 0 && dependency_count != 0);
+    t.dependents--;
+    dependency_count--;
+
+    // text << "U " << t << ": " << t.dependents << " " << dependency_count;
   }
 
   inline void
@@ -118,10 +131,14 @@ namespace build
   inline target_state
   execute (action a, target& t)
   {
-    // This can happen when we re-examine the state after being postponed.
-    //
-    if (t.dependents != 0)
+    if (dependency_count != 0) // Re-examination of a postponed target?
+    {
+      assert (t.dependents != 0);
       t.dependents--;
+      dependency_count--;
+    }
+
+    // text << "E " << t << ": " << t.dependents << " " << dependency_count;
 
     switch (target_state ts = t.state ())
     {
@@ -138,10 +155,10 @@ namespace build
         // clean the group via three of its members, only the last
         // attempt will actually execute the clean. This means that
         // when we match a group member, inside we should also match
-        // the group in order to increment the dependents count.
-        // Though this seems to be a natural requirement (if we
-        // are delegating to the group, we need to find a recipe
-        // for it, just like we would for a prerequisite).
+        // the group in order to increment the dependents count. This
+        // seems to be a natural requirement: if we are delegating to
+        // the group, we need to find a recipe for it, just like we
+        // would for a prerequisite.
         //
         // Note that below we are going to change the group state
         // to postponed. This is not a mistake: until we execute
diff --git a/build/b.cxx b/build/b.cxx
index fea5690..5500c9e 100644
--- a/build/b.cxx
+++ b/build/b.cxx
@@ -744,6 +744,7 @@ main (int argc, char* argv[])
           current_inner_oif = pre_oif;
           current_outer_oif = oif;
           current_mode = pre_oif->mode;
+          dependency_count = 0;
 
           action a (mid, pre_oid, oid);
 
@@ -760,6 +761,7 @@ main (int argc, char* argv[])
         current_inner_oif = oif;
         current_outer_oif = nullptr;
         current_mode = oif->mode;
+        dependency_count = 0;
 
         action a (mid, oid, 0);
 
@@ -777,6 +779,7 @@ main (int argc, char* argv[])
           current_inner_oif = post_oif;
           current_outer_oif = oif;
           current_mode = post_oif->mode;
+          dependency_count = 0;
 
           action a (mid, post_oid, oid);
 
diff --git a/build/context b/build/context
index c8200b7..b8d178b 100644
--- a/build/context
+++ b/build/context
@@ -7,6 +7,7 @@
 
 #include <string>
 #include <ostream>
+#include <cstdint> // uint64_t
 
 #include <butl/filesystem>
 
@@ -33,6 +34,14 @@ namespace build
 
   extern execution_mode current_mode;
 
+  // Total number of dependency relationships in the current action.
+  // Together with the target::dependents count it is incremented
+  // during the rule search & match phase and is decremented during
+  // execution with the expectation of it reaching 0. Used as a sanity
+  // check.
+  //
+  extern std::uint64_t dependency_count;
+
   // Reset the dependency state. In particular, this removes all the
   // targets, scopes, and variable names.
   //
diff --git a/build/context.cxx b/build/context.cxx
index 5a9452b..e8ecf74 100644
--- a/build/context.cxx
+++ b/build/context.cxx
@@ -28,6 +28,7 @@ namespace build
   const operation_info* current_inner_oif;
   const operation_info* current_outer_oif;
   execution_mode current_mode;
+  uint64_t dependency_count;
 
   void
   reset ()
diff --git a/build/install/rule.cxx b/build/install/rule.cxx
index 890228c..f14547c 100644
--- a/build/install/rule.cxx
+++ b/build/install/rule.cxx
@@ -130,7 +130,7 @@ namespace build
         if (pt.state () != target_state::unchanged)
           t.prerequisite_targets.push_back (&pt);
         else
-          pt.dependents--; // No intent to execute, so compensate.
+          unmatch (a, pt); // No intent to execute.
       }
 
       // This is where we diverge depending on the operation. In the
diff --git a/build/operation.cxx b/build/operation.cxx
index 9bfa8d2..694ad1c 100644
--- a/build/operation.cxx
+++ b/build/operation.cxx
@@ -114,12 +114,11 @@ namespace build
   {
     tracer trace ("execute");
 
-    // Build collecting postponed targets (to be re-examined later).
+    // Execute collecting postponed targets (to be re-examined later).
+    // Do it in reverse order if the execution mode is 'last'.
     //
     vector<reference_wrapper<target>> psp;
 
-    // Execute targets in reverse if the execution mode is 'last'.
-    //
     auto body (
       [a, &psp, &trace] (void* v)
       {
@@ -129,11 +128,6 @@ namespace build
 
         switch (execute (a, t))
         {
-        case target_state::postponed:
-          {
-            psp.push_back (t);
-            break;
-          }
         case target_state::unchanged:
           {
             // Be quiet in pre/post operations.
@@ -142,6 +136,9 @@ namespace build
               info << diag_done (a, t);
             break;
           }
+        case target_state::postponed:
+          psp.push_back (t);
+          break;
         case target_state::changed:
           break;
         case target_state::failed:
@@ -156,32 +153,27 @@ namespace build
     else
       for (void* v: reverse_iterate (ts)) body (v);
 
-    // Re-examine postponed targets. Note that we will do it in the
-    // order added, so no need to check the execution mode.
+    // We should have executed every target that we matched.
+    //
+    assert (dependency_count == 0);
+
+    // Re-examine postponed targets. This is the only reliable way to
+    // find out whether the target has changed.
     //
     for (target& t: psp)
     {
-      if (t.state () == target_state::postponed)
-        execute (a, t); // Re-examine the state.
-
-      switch (t.state ())
+      switch (execute (a, t))
       {
-      case target_state::postponed:
-        {
-          info << "unable to " << diag_do (a, t) << " at this time";
-          break;
-        }
       case target_state::unchanged:
         {
-          // Be quiet in pre/post operations.
-          //
           if (a.outer_operation () == 0)
             info << diag_done (a, t);
           break;
         }
-      case target_state::unknown: // Assume something was done to it.
       case target_state::changed:
         break;
+      case target_state::postponed:
+        assert (false);
       case target_state::failed:
         //@@ This could probably happen in a parallel build.
       default:
diff --git a/build/rule.cxx b/build/rule.cxx
index 97cd20f..7bf030d 100644
--- a/build/rule.cxx
+++ b/build/rule.cxx
@@ -193,7 +193,7 @@ namespace build
     // First update prerequisites (e.g. create parent directories)
     // then create this directory.
     //
-    if (t.has_prerequisites ())
+    if (!t.prerequisite_targets.empty ())
       ts = execute_prerequisites (a, t);
 
     const path& d (t.dir); // Everything is in t.dir.
@@ -231,26 +231,16 @@ namespace build
     // The reverse order of update: first delete this directory,
     // then clean prerequisites (e.g., delete parent directories).
     //
-    rmdir_status rs (rmdir (t.dir, t));
-
-    target_state ts (target_state::unchanged);
-    if (t.has_prerequisites ())
-      ts = reverse_execute_prerequisites (a, t);
-
-    // If we couldn't remove the directory, return postponed meaning
-    // that the operation could not be performed at this time.
+    // Don't fail if we couldn't remove the directory because it
+    // is not empty (or is current working directory). In this
+    // case rmdir() will issue a warning when appropriate.
     //
-    switch (rs)
-    {
-    case rmdir_status::success: ts |= target_state::changed; break;
-    case rmdir_status::not_empty:
-      {
-        if (!work.sub (t.dir)) // No use postponing removing working directory.
-          ts |= target_state::postponed;
-        break;
-      }
-    default: break;
-    }
+    target_state ts (rmdir (t.dir, t)
+                     ? target_state::changed
+                     : target_state::unchanged);
+
+    if (!t.prerequisite_targets.empty ())
+      ts |= reverse_execute_prerequisites (a, t);
 
     return ts;
   }
diff --git a/build/target b/build/target
index 2aa3b5f..dd58b24 100644
--- a/build/target
+++ b/build/target
@@ -41,14 +41,14 @@ namespace build
   //
   enum class target_state: std::uint8_t
   {
-    // The order of the enumerators is arranged so that their
-    // inegral values indicate whether one "overrides" the other
-    // in the merge operator (see below).
+    // The order of the enumerators is arranged so that their integral
+    // values indicate whether one "overrides" the other in the merge
+    // operator (see below).
     //
     unknown,
     unchanged,
-    changed,
     postponed,
+    changed,
     failed,
     group       // Target's state is the group's state.
   };
@@ -68,22 +68,15 @@ namespace build
   // Recipe.
   //
   // The returned target state should be changed, unchanged, or
-  // postponed. If there is an error, then the recipe should throw
-  // rather than returning failed.
+  // postponed, though you shouldn't be returning postponed
+  // directly. If there is an error, then the recipe should
+  // throw rather than returning failed.
   //
-  // The recipe execution protocol is as follows: before executing
-  // the recipe, the caller sets the target's state to failed. If
-  // the recipe returns normally and the target's state is still
-  // failed, then the caller sets it to the returned value. This
-  // means that the recipe can set the target's state manually to
-  // some other value. For example, setting it to unknown will
-  // result in the recipe to be executed again if this target is a
-  // prerequisite of another target. Note that in this case the
-  // returned by the recipe value is still used (by the caller) as
-  // the resulting target state for this execution of the recipe.
-  // Returning postponed from the last call to the recipe means
-  // that the action could not be executed at this time (see fsdir
-  // clean for an example).
+  // The return value of the recipe is used to update the target
+  // state except if the state was manually set by the recipe to
+  // target_state::group. Note that in this case the returned by
+  // the recipe value is still used as the resulting target state
+  // so it should match the group's state.
   //
   using recipe_function = target_state (action, target&);
   using recipe = std::function<recipe_function>;
diff --git a/build/target.cxx b/build/target.cxx
index cf8747f..033a7f9 100644
--- a/build/target.cxx
+++ b/build/target.cxx
@@ -32,7 +32,7 @@ namespace build
   // target_state
   //
   static const char* target_state_[] = {
-    "unknown", "unchanged", "changed", "postponed", "failed", "group"};
+    "unknown", "unchanged", "postponed", "changed", "failed", "group"};
 
   ostream&
   operator<< (ostream& os, target_state ts)
diff --git a/build/test/rule.cxx b/build/test/rule.cxx
index 6e82d8d..30de93c 100644
--- a/build/test/rule.cxx
+++ b/build/test/rule.cxx
@@ -210,7 +210,10 @@ namespace build
           build::match (a, *it);
 
           if (it->state () == target_state::unchanged)
+          {
+            unmatch (a, *it);
             it = nullptr;
+          }
         }
 
         if (ot != nullptr && in == on)
@@ -218,7 +221,10 @@ namespace build
           build::match (a, *ot);
 
           if (ot->state () == target_state::unchanged)
+          {
+            unmatch (a, *ot);
             ot = nullptr;
+          }
         }
         else
           ot = it;