Implement short-circuiting to group state

This is necessary to get rid of bogus restarts in inject_prerequisites()
where it think a group member was updated since its state changed from
unknown to (group's) changed.

7 files changed, 90 insertions, 65 deletions

diff --git a/build2/algorithm.ixx b/build2/algorithm.ixx
index 3930412..17c9d8d 100644
--- a/build2/algorithm.ixx
+++ b/build2/algorithm.ixx
@@ -135,11 +135,14 @@ namespace build2
     if (dependency_count != 0) // Re-examination of a postponed target?
     {
       assert (t.dependents != 0);
-      t.dependents--;
-      dependency_count--;
+      --t.dependents;
+      --dependency_count;
     }
 
-    switch (target_state ts = t.state ())
+    // Don't short-circuit to the group state since we need to execute the
+    // member's recipe to keep the dependency counts straight.
+    //
+    switch (target_state ts = t.state (false))
     {
     case target_state::unchanged:
     case target_state::changed:
@@ -188,6 +191,8 @@ namespace build2
   inline target_state
   execute_direct (action a, target& t)
   {
+    // Here we don't care about the counts so short-circuit state is ok.
+    //
     switch (target_state ts = t.state ())
     {
     case target_state::unchanged:
diff --git a/build2/bin/target.cxx b/build2/bin/target.cxx
index b0ce6d5..6cd5d11 100644
--- a/build2/bin/target.cxx
+++ b/build2/bin/target.cxx
@@ -196,6 +196,8 @@ namespace build2
     {
       // Don't clear prerequisite_targets since it is "given" to our
       // members to implement "library meta-information protocol".
+      //
+      raw_state = target_state::unknown;
     }
 
     static target*
diff --git a/build2/cli/rule.cxx b/build2/cli/rule.cxx
index 65e940b..d986eca 100644
--- a/build2/cli/rule.cxx
+++ b/build2/cli/rule.cxx
@@ -108,13 +108,13 @@ namespace build2
         //
         target& t (xt);
 
-        // First see if we are already linked-up to the cli.cxx{} group.
-        // If it is some other group, then we are definitely not a match.
+        // First see if we are already linked-up to the cli.cxx{} group. If
+        // it is some other group, then we are definitely not a match.
         //
         if (t.group != nullptr)
           return t.group->is_a<cli_cxx> ();
 
-        // Then check if there is a corresponding cli.cxx{} group.
+        // Check if there is a corresponding cli.cxx{} group.
         //
         cli_cxx* g (targets.find<cli_cxx> (t.dir, t.out, t.name));
 
@@ -147,8 +147,8 @@ namespace build2
 
         if (g != nullptr)
         {
-          // Resolve the group's members. This should link us up to
-          // the group.
+          // Resolve the group's members. This should link us up to the
+          // group.
           //
           resolve_group_members (a, *g);
 
diff --git a/build2/cxx/compile.cxx b/build2/cxx/compile.cxx
index b7577d4..a7986c5 100644
--- a/build2/cxx/compile.cxx
+++ b/build2/cxx/compile.cxx
@@ -571,7 +571,9 @@ namespace build2
 
           if (ns != os && ns != target_state::unchanged)
           {
-            l6 ([&]{trace << "updated " << pt;});
+            l6 ([&]{trace << "updated " << pt
+                          << "; old state " << os
+                          << "; new state " << ns;});
             return true;
           }
         }
@@ -588,8 +590,8 @@ namespace build2
         return false;
       };
 
-      // Update and add header file to the list of prerequisite targets.
-      // Depending on the cache flag, the file is assumed to either have comes
+      // Update and add a header file to the list of prerequisite targets.
+      // Depending on the cache flag, the file is assumed to either have come
       // from the depdb cache or from the compiler run. Return whether the
       // extraction process should be restarted.
       //
diff --git a/build2/target b/build2/target
index ac67ff8..76d7132 100644
--- a/build2/target
+++ b/build2/target
@@ -59,16 +59,15 @@ namespace build2
 
   // Recipe.
   //
-  // The returned target state should be changed, unchanged, or
-  // postponed, though you shouldn't be returning postponed
-  // directly. If there is an error, then the recipe should
-  // throw rather than returning failed.
+  // The returned target state should be changed, unchanged, or postponed,
+  // though you shouldn't be returning postponed directly. If there is an
+  // error, then the recipe should throw rather than returning failed.
   //
-  // 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.
+  // The return value of the recipe is used to update the target state except
+  // if the state is target_state::group (either was was manually set by the
+  // recipe or already that value). 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 = function<recipe_function>;
@@ -146,44 +145,45 @@ namespace build2
     const string name;
     const string* ext;  // Extension. NULL - unspecified, empty - no extension.
 
-    // Target group to which this target belongs, if any. Note that
-    // we assume that the group and all its members are in the same
-    // scope (for example, in variable lookup). We also don't support
-    // nested groups.
+    // Target group to which this target belongs, if any. Note that we assume
+    // that the group and all its members are in the same scope (for example,
+    // in variable lookup). We also don't support nested groups.
     //
-    // The semantics of the interaction between the group and its
-    // members and what it means to, say, update the group, is
-    // unspecified and determined by the group's type. In particular,
-    // a group can be created out of member types that have no idea
-    // they are part of this group (e.g., cli.cxx{}).
+    // The semantics of the interaction between the group and its members and
+    // what it means to, say, update the group, is unspecified and is
+    // determined by the group's type. In particular, a group can be created
+    // out of member types that have no idea they are part of this group
+    // (e.g., cli.cxx{}).
     //
-    // Normally, however, there are two kinds of groups: "alternatives"
-    // and "combination". In an alternatives group, normally one of the
-    // members is selected when the group is mentioned as a prerequisite
-    // with, perhaps, an exception for special rules, like aliases, where
-    // it makes more sense to treat the group as a whole. In this case we
-    // say that the rule "semantically recognizes" the group and picks
-    // some of its members.
+    // Normally, however, there are two kinds of groups: "alternatives" and
+    // "combination". In an alternatives group, normally one of the members is
+    // selected when the group is mentioned as a prerequisite with, perhaps,
+    // an exception for special rules, like aliases, where it makes more sense
+    // to treat the group as a whole. In this case we say that the rule
+    // "semantically recognizes" the group and picks some of its members.
     //
-    // Updating an alternatives group as a whole can mean updating some
-    // subset of its members (e.g., lib{}). Or the group may not support
-    // this at all (e.g., obj{}).
+    // Updating an alternatives group as a whole can mean updating some subset
+    // of its members (e.g., lib{}). Or the group may not support this at all
+    // (e.g., obj{}).
     //
-    // In a combination group, when a group is updated, normally all
-    // members are updates (and usually with a single command), though
-    // there could be some members that are omitted, depending on the
-    // configuration (e.g., an inline file not/being generated). When
-    // a combination group is mentioned as a prerequisite, the rule
-    // is usually interested in the individual members rather than
-    // the whole group. For example, a C++ compile rule would like to
-    // "see" the ?xx{} members when it gets a cli.cxx{} group.
+    // In a combination group, when a group is updated, normally all members
+    // are updates (and usually with a single command), though there could be
+    // some members that are omitted, depending on the configuration (e.g., an
+    // inline file not/being generated). When a combination group is mentioned
+    // as a prerequisite, the rule is usually interested in the individual
+    // members rather than the whole group. For example, a C++ compile rule
+    // would like to "see" the ?xx{} members when it gets a cli.cxx{} group.
     //
-    // Which brings us to the group iteration mode. The target type
-    // contains a member called see_through that indicates whether the
-    // default iteration mode for the group should be "see through";
-    // that is, whether we see the members or the group itself. For
-    // the iteration support itself, see the *_prerequisite_members()
-    // machinery below.
+    // Which brings us to the group iteration mode. The target type contains a
+    // member called see_through that indicates whether the default iteration
+    // mode for the group should be "see through"; that is, whether we see the
+    // members or the group itself. For the iteration support itself, see the
+    // *_prerequisite_members() machinery below.
+    //
+    // In a combination group we usually want the state (and timestamp; see
+    // mtime()) for members to come from the group. This is achieved with the
+    // special target_state::group state. You would normally also use the
+    // group_recipe for group members.
     //
     target* group = nullptr;
 
@@ -197,8 +197,9 @@ namespace build2
     target (dir_path d, dir_path o, string n, const string* e)
         : dir (move (d)), out (move (o)), name (move (n)), ext (e) {}
 
-    // Reset the target before matching a rule for it. The default
-    // implementation clears prerequisite_targets.
+    // Reset the target before matching it to a rule. The default
+    // implementation clears prerequisite_targets and sets the state to
+    // unknown.
     //
     virtual void
     reset (action_type);
@@ -353,10 +354,27 @@ namespace build2
   public:
     target_state raw_state = target_state::unknown;
 
+    // By default we go an extra step and short-circuit to the target state
+    // even if the raw state is not group provided the recipe is group_recipe.
+    // This is normally what you want or need, as in inject_prerequisites()
+    // in the cxx module. But sometimes not, as in execute().
+    //
     target_state
-    state () const
+    state (bool shortcircuit = true) const
     {
-      return raw_state != target_state::group ? raw_state : group->raw_state;
+      if (raw_state == target_state::group)
+        return group->raw_state;
+
+      if (group == nullptr || !shortcircuit)
+        return raw_state;
+
+      if (recipe_function* const* f = recipe_.target<recipe_function*> ())
+      {
+        if (*f == &group_action)
+          return group->raw_state;
+      }
+
+      return raw_state;
     }
 
     // Number of direct targets that depend on this target in the current
diff --git a/build2/target.cxx b/build2/target.cxx
index 663d31d..4d4fe2a 100644
--- a/build2/target.cxx
+++ b/build2/target.cxx
@@ -66,12 +66,9 @@ namespace build2
     action = a;
     recipe_ = move (r);
 
-    // Also reset the target state. If this is a noop recipe, then
-    // mark the target unchanged so that we don't waste time executing
-    // the recipe.
+    // If this is a noop recipe, then mark the target unchanged so that we
+    // don't waste time executing the recipe.
     //
-    raw_state = target_state::unknown;
-
     if (recipe_function** f = recipe_.target<recipe_function*> ())
     {
       if (*f == &noop_action)
@@ -89,6 +86,7 @@ namespace build2
   reset (action_type)
   {
     prerequisite_targets.clear ();
+    raw_state = target_state::unknown;
   }
 
   group_view target::
diff --git a/tests/cli/simple/buildfile b/tests/cli/simple/buildfile
index 998eb67..47e06d9 100644
--- a/tests/cli/simple/buildfile
+++ b/tests/cli/simple/buildfile
@@ -1,8 +1,8 @@
 using cxx
 
-hxx.ext =
-cxx.ext = cpp
-ixx.ext = ipp
+hxx{*}: extension =
+cxx{*}: extension = cpp
+ixx{*}: extension = ipp
 
 cxx.poptions += -I$out_root