From 26750bd14c715bb616d1779f031b8f5b0ced1bbf Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Tue, 8 Jun 2021 15:30:35 +0200 Subject: Redo low verbosity diagnostic deduction to use scope instead of target --- libbuild2/adhoc-rule-buildscript.cxx | 10 ++---- libbuild2/adhoc-rule-buildscript.hxx | 3 +- libbuild2/adhoc-rule-cxx.cxx | 2 +- libbuild2/adhoc-rule-cxx.hxx | 3 +- libbuild2/build/script/parser.cxx | 30 +++++++--------- libbuild2/build/script/parser.hxx | 16 +++++++-- libbuild2/build/script/parser.test.cxx | 2 +- libbuild2/parser.cxx | 10 ++---- libbuild2/rule.hxx | 13 +++---- tests/dependency/recipe/testscript | 65 +++++++++++----------------------- 10 files changed, 58 insertions(+), 96 deletions(-) diff --git a/libbuild2/adhoc-rule-buildscript.cxx b/libbuild2/adhoc-rule-buildscript.cxx index c4b9169..c715ff2 100644 --- a/libbuild2/adhoc-rule-buildscript.cxx +++ b/libbuild2/adhoc-rule-buildscript.cxx @@ -23,11 +23,7 @@ using namespace std; namespace build2 { bool adhoc_buildscript_rule:: - recipe_text (context& ctx, - const scope& s, - const target* tg, - string&& t, - attributes& as) + recipe_text (const scope& s, string&& t, attributes& as) { // Handle and erase recipe-specific attributes. // @@ -58,9 +54,9 @@ namespace build2 checksum = sha256 (t).string (); istringstream is (move (t)); - build::script::parser p (ctx); + build::script::parser p (s.ctx); - script = p.pre_parse (s, tg, actions, + script = p.pre_parse (s, actions, is, loc.file, loc.line + 1, move (diag), as.loc); diff --git a/libbuild2/adhoc-rule-buildscript.hxx b/libbuild2/adhoc-rule-buildscript.hxx index a04840d..748a4c4 100644 --- a/libbuild2/adhoc-rule-buildscript.hxx +++ b/libbuild2/adhoc-rule-buildscript.hxx @@ -42,8 +42,7 @@ namespace build2 : adhoc_rule (move (n), l, b) {} virtual bool - recipe_text (context&, const scope&, const target*, - string&&, attributes&) override; + recipe_text (const scope&, string&&, attributes&) override; virtual void dump_attributes (ostream&) const override; diff --git a/libbuild2/adhoc-rule-cxx.cxx b/libbuild2/adhoc-rule-cxx.cxx index 5576eda..83eeee0 100644 --- a/libbuild2/adhoc-rule-cxx.cxx +++ b/libbuild2/adhoc-rule-cxx.cxx @@ -40,7 +40,7 @@ namespace build2 } bool adhoc_cxx_rule:: - recipe_text (context&, const scope&, const target*, string&& t, attributes&) + recipe_text (const scope&, string&& t, attributes&) { code = move (t); return true; diff --git a/libbuild2/adhoc-rule-cxx.hxx b/libbuild2/adhoc-rule-cxx.hxx index 8254906..0ca6038 100644 --- a/libbuild2/adhoc-rule-cxx.hxx +++ b/libbuild2/adhoc-rule-cxx.hxx @@ -72,8 +72,7 @@ namespace build2 optional sep); virtual bool - recipe_text (context&, const scope&, const target*, - string&&, attributes&) override; + recipe_text (const scope&, string&&, attributes&) override; virtual ~adhoc_cxx_rule () override; diff --git a/libbuild2/build/script/parser.cxx b/libbuild2/build/script/parser.cxx index 9194e30..b602880 100644 --- a/libbuild2/build/script/parser.cxx +++ b/libbuild2/build/script/parser.cxx @@ -28,7 +28,6 @@ namespace build2 script parser:: pre_parse (const scope& bs, - const target* tg, const small_vector& as, istream& is, const path_name& pn, uint64_t line, optional diag, const location& diag_loc) @@ -40,11 +39,11 @@ namespace build2 lexer l (is, *path_, line, lexer_mode::command_line); set_lexer (&l); - // The script shouldn't be able to modify the target/scopes. + // The script shouldn't be able to modify the scopes. // - target_ = const_cast (tg); + target_ = nullptr; actions_ = &as; - scope_ = const_cast (tg != nullptr ? &tg->base_scope () : &bs); + scope_ = const_cast (&bs); root_ = scope_->root_scope (); pbase_ = scope_->src_path_; @@ -585,18 +584,12 @@ namespace build2 parse_names_result pr; { - // During pre-parse, if the script name is not set manually and we - // have the target, we suspend pre-parse, parse the command names - // for real and try to deduce the script name from the result. - // Otherwise, we continue to pre-parse and bail out after parsing - // the names. - // - // @@ TODO: maybe we could recognize literal names even if target - // is NULL (see the tests for some ugly recipes). But will need - // to be careful to still pick up ambiguity between literal and - // skipped due to target being NULL. + // During pre-parse, if the script name is not set manually we + // suspend pre-parse, parse the command names for real and try to + // deduce the script name from the result. Otherwise, we continue + // to pre-parse and bail out after parsing the names. // - // Note that the later is not just an optimization since expansion + // Note that the latter is not just an optimization since expansion // that wouldn't fail during execution may fail in this special // mode, for example: // @@ -615,7 +608,7 @@ namespace build2 // // This is also the reason why we add a diag frame. // - if (pre_parse_ && (diag_weight_ != 4 && target_ != nullptr)) + if (pre_parse_ && diag_weight_ != 4) { pre_parse_ = false; // Make parse_names() perform expansions. pre_parse_suspended_ = true; @@ -646,7 +639,7 @@ namespace build2 pre_parse_ = true; } - if (pre_parse_ && (diag_weight_ == 4 || target_ == nullptr)) + if (pre_parse_ && diag_weight_ == 4) return nullopt; } @@ -1041,6 +1034,7 @@ namespace build2 // it could be used for (we need scope_ for calling functions such as // $target.path()). // + target_ = nullptr; root_ = const_cast (&rs); scope_ = const_cast (&bs); pbase_ = scope_->src_path_; @@ -1170,7 +1164,7 @@ namespace build2 const variable* pvar (scope_->ctx.var_pool.find (name)); if (pvar != nullptr) - r = (*target_)[*pvar]; + r = (*scope_)[*pvar]; } if (!depdb_clear_) diff --git a/libbuild2/build/script/parser.hxx b/libbuild2/build/script/parser.hxx index 8c787f9..948c381 100644 --- a/libbuild2/build/script/parser.hxx +++ b/libbuild2/build/script/parser.hxx @@ -32,11 +32,21 @@ namespace build2 parser (context& c): build2::script::parser (c) {} // Note that the returned script object references the passed path - // name. Target is NULL if this recipe is shared among multiple - // targets. + // name. + // + // Note also that we use the scope to lookup variable values while + // trying to deduce the low verbosity diagnostics name (see code + // around pre_parse_suspended for details). But that means we may + // derive such a name based on the wrong value. This can happen if the + // expanded variable value is reset after the recipe has been + // pre-parsed or if such a value is set on the target (which is where + // we start when looking up variables during the real parse). The + // current thinking is that a remote possibility of this happening is + // acceptable in this situation -- the worst that can happen is that + // we will end up with mismatching diagnostics. // script - pre_parse (const scope&, const target*, const small_vector&, + pre_parse (const scope&, const small_vector&, istream&, const path_name&, uint64_t line, optional diag_name, const location& diag_loc); diff --git a/libbuild2/build/script/parser.test.cxx b/libbuild2/build/script/parser.test.cxx index 0bc1c5d..c9356a8 100644 --- a/libbuild2/build/script/parser.test.cxx +++ b/libbuild2/build/script/parser.test.cxx @@ -208,7 +208,7 @@ namespace build2 parser p (ctx); path_name nm ("buildfile"); - script s (p.pre_parse (tt.base_scope (), &tt, acts, + script s (p.pre_parse (tt.base_scope (), acts, cin, nm, 11 /* line */, (m != mode::diag diff --git a/libbuild2/parser.cxx b/libbuild2/parser.cxx index 6f59b30..d0661a9 100644 --- a/libbuild2/parser.cxx +++ b/libbuild2/parser.cxx @@ -1567,7 +1567,6 @@ namespace build2 } } - bool multi (replay_ != replay::stop); // Multiple targets. bool first (replay_ != replay::play); // First target. bool clean (false); // Seen recipe that requires cleanup. @@ -1584,14 +1583,13 @@ namespace build2 { const string& name; small_vector, 1>& recipes; - bool multi; bool first; bool& clean; size_t i; attributes& as; buildspec& bs; const location& bsloc; - } d {name, recipes, multi, first, clean, i, as, bs, bsloc}; + } d {name, recipes, first, clean, i, as, bs, bsloc}; // Note that this function must be called at most once per iteration. // @@ -1799,11 +1797,7 @@ namespace build2 // Set the recipe text. // - if (ar.recipe_text (ctx, - *scope_, - d.multi ? nullptr : target_, - move (t.value), - d.as)) + if (ar.recipe_text (*scope_, move (t.value), d.as)) d.clean = true; // Verify we have no unhandled attributes. diff --git a/libbuild2/rule.hxx b/libbuild2/rule.hxx index 67c0f6d..e46c402 100644 --- a/libbuild2/rule.hxx +++ b/libbuild2/rule.hxx @@ -172,18 +172,13 @@ namespace build2 // Set the rule text, handle any recipe-specific attributes, and return // true if the recipe builds anything in the build/recipes/ directory and - // therefore requires cleanup. + // therefore requires cleanup. Scope is the scope of the recipe. // - // Target is not NULL only if this recipe is for a single target. Scope is - // the scope of the recipe (not necessarily the same as the target's base - // scope). - // - // Note that this function is called after the actions member has been - // populated. + // Note also that this function is called after the actions member has + // been populated. // virtual bool - recipe_text (context&, const scope&, const target*, - string&&, attributes&) = 0; + recipe_text (const scope&, string&&, attributes&) = 0; public: // Some of the operations come in compensating pairs, such as update and diff --git a/tests/dependency/recipe/testscript b/tests/dependency/recipe/testscript index 3126691..ebcdfdb 100644 --- a/tests/dependency/recipe/testscript +++ b/tests/dependency/recipe/testscript @@ -28,16 +28,14 @@ EOE $* <>/~%EOE% alias{x y}: alias{z} {{ - diag echo echo }} dump alias{y} EOI -:6:1: dump: +:5:1: dump: % .+/alias\{y\}: .+/:alias\{z\}% - % perform(update) + % [diag=echo] perform(update) {{ - diag echo echo }} EOE @@ -66,16 +64,14 @@ $* <>/~%EOE% alias{x y}: % perform(update clean) {{ - diag echo echo }} dump alias{y} EOI -:7:1: dump: +:6:1: dump: % .+/alias\{y\}:% - % perform(update) perform(clean) + % [diag=echo] perform(update) perform(clean) {{ - diag echo echo }} EOE @@ -128,19 +124,17 @@ alias{x y}: alias{z} var = x } {{ - diag echo echo }} dump alias{y} EOI -:9:1: dump: +:8:1: dump: % .+/alias\{y\}: .+/:alias\{z\}% { var = x } - % perform(update) + % [diag=echo] perform(update) {{ - diag echo echo }} EOE @@ -176,19 +170,17 @@ $* <>/~%EOE% var = x } {{ - diag echo echo }} dump alias{y} EOI -:9:1: dump: +:8:1: dump: % .+/alias\{y\}: .+/:alias\{z\}% { var = x } - % perform(update) + % [diag=echo] perform(update) {{ - diag echo echo }} EOE @@ -226,19 +218,17 @@ alias{x y}: } % perform(update) {{ - diag echo echo }} dump alias{y} EOI -:10:1: dump: +:9:1: dump: % .+/alias\{y\}:% { var = x } - % perform(update) + % [diag=echo] perform(update) {{ - diag echo echo }} EOE @@ -273,26 +263,22 @@ EOE $* <>/~%EOE% alias{x y}: alias{z} {{ - diag echo echo }} % clean {{{ - diag cat cat }}} dump alias{y} EOI -:11:1: dump: +:9:1: dump: % .+/alias\{y\}: .+/:alias\{z\}% - % perform(update) + % [diag=echo] perform(update) {{ - diag echo echo }} - % perform(clean) + % [diag=cat] perform(clean) {{{ - diag cat cat }}} EOE @@ -331,27 +317,23 @@ alias{x y}: % clean {{ - diag echo echo }} % update {{{ - diag cat cat }}} dump alias{y} EOI -:14:1: dump: +:12:1: dump: % .+/alias\{y\}:% - % perform(clean) + % [diag=echo] perform(clean) {{ - diag echo echo }} - % perform(update) + % [diag=cat] perform(update) {{{ - diag cat cat }}} EOE @@ -504,21 +486,18 @@ alias{x y}: alias{z} % if $f {{ - diag false false }} else {{ - diag echo echo }} dump alias{y} EOI -:14:1: dump: +:12:1: dump: % .+/alias\{y\}: .+/:alias\{z\}% - % perform(update) + % [diag=echo] perform(update) {{ - diag echo echo }} EOE @@ -580,27 +559,23 @@ switch $f { case 1 {{ - diag false false }} case 2 {{ - diag echo echo }} default {{ - diag false false }} } dump alias{y} EOI -:22:1: dump: +:19:1: dump: % .+/alias\{y\}: .+/:alias\{z\}% - % perform(update) + % [diag=echo] perform(update) {{ - diag echo echo }} EOE -- cgit v1.1