From 6082d76936b8a65380eb7af03b4167d8f0298158 Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Thu, 21 Apr 2016 12:18:15 +0200 Subject: 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. --- build2/algorithm.ixx | 11 +++-- build2/bin/target.cxx | 2 + build2/cli/rule.cxx | 10 ++--- build2/cxx/compile.cxx | 8 ++-- build2/target | 110 ++++++++++++++++++++++++++------------------- build2/target.cxx | 8 ++-- tests/cli/simple/buildfile | 6 +-- 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 (); - // Then check if there is a corresponding cli.cxx{} group. + // Check if there is a corresponding cli.cxx{} group. // cli_cxx* g (targets.find (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; @@ -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 ()) + { + 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 ()) { 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 -- cgit v1.1