From 6883355bc8291d740e4c86c3e15b61f227977fd1 Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Thu, 8 Feb 2018 09:18:25 +0200 Subject: Update/cleanup comment documentation for inner/outer operation semantics --- build2/algorithm.cxx | 4 ++-- build2/cc/link-rule.cxx | 4 ++-- build2/install/rule.cxx | 14 +++++++------- build2/operation.hxx | 45 ++++++++++++++++++++++++++++++++------------- build2/target.hxx | 2 +- build2/test/rule.cxx | 10 +++++----- 6 files changed, 49 insertions(+), 30 deletions(-) diff --git a/build2/algorithm.cxx b/build2/algorithm.cxx index 58ddb24..d39711b 100644 --- a/build2/algorithm.cxx +++ b/build2/algorithm.cxx @@ -222,10 +222,10 @@ namespace build2 const rule_match* match_impl (action a, target& t, const rule* skip, bool try_match) { - // If this is an outer operation (Y_for_X), then we look for rules + // If this is an outer operation (Y-for-X), then we look for rules // registered for the outer id (X). Note that we still pass the original // action to the rule's match() function so that it can distinguish - // between a pre/post operation (Y_for_X) and the actual operation (X). + // between a pre/post operation (Y-for-X) and the actual operation (X). // meta_operation_id mo (a.meta_operation ()); operation_id o (a.inner () ? a.operation () : a.outer_operation ()); diff --git a/build2/cc/link-rule.cxx b/build2/cc/link-rule.cxx index b6b707d..cd355c6 100644 --- a/build2/cc/link-rule.cxx +++ b/build2/cc/link-rule.cxx @@ -290,7 +290,7 @@ namespace build2 file& t (xt.as ()); - // Note that for_install is signalled by install_rule and therefore + // Note that for-install is signalled by install_rule and therefore // can only be relied upon during execute. // match_data& md (t.data (match_data ())); @@ -449,7 +449,7 @@ namespace build2 // // Note that we do it here regardless of whether we are installing // or not for two reasons. Firstly, it is not easy to detect this - // situation in apply() since the for_install hasn't yet been + // situation in apply() since the for-install hasn't yet been // communicated by install_rule. Secondly, always having the member // takes care of cleanup automagically. The actual generation // happens in perform_update() below. diff --git a/build2/install/rule.cxx b/build2/install/rule.cxx index 8bf58aa..00166c2 100644 --- a/build2/install/rule.cxx +++ b/build2/install/rule.cxx @@ -45,9 +45,9 @@ namespace build2 { // We always match. // - // Note that we are called both as the outer part during the "update for - // un/install" pre-operation and as the inner part during the - // un/install operation itself. + // Note that we are called both as the outer part during the update-for- + // un/install pre-operation and as the inner part during the un/install + // operation itself. // return true; } @@ -234,12 +234,12 @@ namespace build2 { tracer trace ("install::file_rule::apply"); - // Note that we are called both as the outer part during the "update for - // un/install" pre-operation and as the inner part during the - // un/install operation itself. + // Note that we are called both as the outer part during the update-for- + // un/install pre-operation and as the inner part during the un/install + // operation itself. // // In both cases we first determine if the target is installable and - // return noop if it's not. Otherwise, in the first case (update for + // return noop if it's not. Otherwise, in the first case (update-for- // un/install) we delegate to the normal update and in the second // (un/install) -- perform the test. // diff --git a/build2/operation.hxx b/build2/operation.hxx index 7f59783..3f812d5 100644 --- a/build2/operation.hxx +++ b/build2/operation.hxx @@ -34,19 +34,38 @@ namespace build2 using operation_id = uint8_t; using action_id = uint8_t; - // Meta-operations and operations are not the end of the story. We - // also have operation nesting (currently only one level deep) which - // is used to implement pre/post operations (currently, but may be - // useful for other things). Here is the idea: the test operation - // needs to make sure that the targets that it needs to test are - // up-to-date. So it runs update as its pre-operation. It is almost - // like an ordinary update except that it has test as its outer - // operation (the meta-operations are always the same). This way a - // rule can recognize that this is "update for test" and do something - // differently. For example, if an executable is not a test, then - // there is no use updating it. At the same time, most rules will - // ignore the fact that this is a nested update and for them it is - // "update as usual". + // Meta-operations and operations are not the end of the story. We also have + // operation nesting (currently only one level deep) which is used to + // implement pre/post operations (currently, but may be useful for other + // things). Here is the idea: the test operation needs to make sure that the + // targets that it needs to test are up-to-date. So it runs update as its + // pre-operation. It is almost like an ordinary update except that it has + // test as its outer operation (the meta-operations are always the same). + // This way a rule can recognize that this is "update for test" and do + // something differently. For example, if an executable is not a test, then + // there is no use updating it. At the same time, most rules will ignore the + // fact that this is a nested update and for them it is "update as usual". + // + // This inner/outer operation support is implemented by maintaining two + // independent "target states" (see target::state; initially we tried to do + // it via rule/recipe override but that didn't end up well, to put it + // mildly). While the outer operation normally "directs" the inner, inner + // rules can still be matched/executed directly, without outer's involvement + // (e.g., because of other inner rules). A typical implementation of an + // outer rule either returns noop or delegates to the inner rule. In + // particular, it should not replace or override the inner's logic. + // + // While most of the relevant target state is duplicated, certain things are + // shared among the inner/outer rules, such as the target data pad and the + // group state. In particular, it is assumed the group state is always + // determined by the inner rule (see resolve_group_members()). + // + // Normally, an outer rule will be responsible for any additional, outer + // operation-specific work. Sometimes, however, the inner rule needs to + // customize its behavior. In this case the outer and inner rules must + // communicate this explicitly (normally via the target's data pad) and + // there is a number of restrictions to this approach. See + // cc::{link,install}_rule for details. // struct action { diff --git a/build2/target.hxx b/build2/target.hxx index 405bf4f..23213a2 100644 --- a/build2/target.hxx +++ b/build2/target.hxx @@ -441,7 +441,7 @@ namespace build2 static size_t count_executed () {return offset_executed + count_base ();} static size_t count_busy () {return offset_busy + count_base ();} - // Inner/outer operation state. + // Inner/outer operation state. See operation.hxx for details. // struct opstate { diff --git a/build2/test/rule.cxx b/build2/test/rule.cxx index a42ceca..7f6a133 100644 --- a/build2/test/rule.cxx +++ b/build2/test/rule.cxx @@ -35,8 +35,8 @@ namespace build2 recipe rule:: apply (action a, target& t) const { - // Note that we are called both as the outer part during the "update for - // test" pre-operation and as the inner part during the test operation + // Note that we are called both as the outer part during the update-for- + // test pre-operation and as the inner part during the test operation // itself. // // In both cases we first determine if the target is testable and return @@ -52,7 +52,7 @@ namespace build2 // // test'able | pass'able | neither // | | - // update for test delegate (& pass) | pass | noop + // update-for-test delegate (& pass) | pass | noop // ---------------------------------------+-------------+--------- // test test (& pass) | pass | noop // @@ -150,7 +150,7 @@ namespace build2 // execute them relying on update to assign their paths. // // Causing update for test inputs/scripts is tricky: we cannot - // match for update_for_install because this same rule will match + // match for update-for-install because this same rule will match // and since the target is not testable, it will return the noop // recipe. // @@ -160,7 +160,7 @@ namespace build2 // iffy, it does make sense: the outer rule we would have matched // would have simply delegated to the inner so we might as well // take a shortcut. The only potential drawback of this approach - // is that we won't be able to provide any for_test customizations + // is that we won't be able to provide any for-test customizations // when updating test inputs/scripts. But such a need seems rather // far fetched. // -- cgit v1.1