From cbcb0b03501ce346ca3778624dcf908e851e6e2e Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Thu, 19 Mar 2020 09:27:22 +0200 Subject: Tweak lookup_config() semantics some more --- libbuild2/config/utility.cxx | 28 +++++++++++++++++++++++++--- libbuild2/config/utility.hxx | 14 +++++++++++--- libbuild2/scope.cxx | 16 +++++++++------- libbuild2/scope.hxx | 24 ++++++++++++++++++++++-- libbuild2/variable.hxx | 12 ++++++++---- libbuild2/variable.txx | 14 +++++++------- 6 files changed, 82 insertions(+), 26 deletions(-) diff --git a/libbuild2/config/utility.cxx b/libbuild2/config/utility.cxx index cf0f73b..b3f94be 100644 --- a/libbuild2/config/utility.cxx +++ b/libbuild2/config/utility.cxx @@ -33,9 +33,31 @@ namespace build2 if (var.overrides != nullptr) { - pair ovr (rs.lookup_override (var, move (org))); - - if (l != ovr.first) // Overriden? + // This is tricky: if we didn't find the original, pretend we have set + // the default value for the purpose of override lookup in order to + // have consistent semantics with the default value case (see notes in + // that implementation for background). + // + // In particular, this makes sure we can first do the lookup without + // the default value and then, if there is no value, call the version + // with the default value and end up with the same result if we called + // the default value version straight away. + // + // Note that we need to detect both when the default value is not + // overridden as well as when the override is based on it (e.g., via + // append; think config.cxx+=-m32). + // + // @@ Maybe a callback that computes the default value on demand is a + // better way? + // + variable_map::value_data v; // NULL value, but must be with version. + if (!l.defined ()) + org = make_pair (lookup (v, var, rs), 1); // As default value case. + + scope::override_info li (rs.lookup_override_info (var, move (org))); + pair& ovr (li.lookup); + + if (l.defined () ? l != ovr.first : !li.original) // Overriden? { // Override is always treated as new. // diff --git a/libbuild2/config/utility.hxx b/libbuild2/config/utility.hxx index bf4728f..cbfe588 100644 --- a/libbuild2/config/utility.hxx +++ b/libbuild2/config/utility.hxx @@ -91,10 +91,16 @@ namespace build2 // // Unlike the rest of the lookup_config() versions, this one leaves the // unspecified value as undefined rather than setting it to a default - // value. This can be useful when we don't have a default value or if we - // want the mentioning of the variable to be omitted from persistent + // value. This can be useful when we don't have a default value or in case + // we want the mentioning of the variable to be omitted from persistent // storage (e.g., a config file) if the default value is used. // + // Note also that we can first do the lookup without the default value and + // then, if there is no value, call the version with the default value and + // end up with the same result if we called the default value version + // straight away. This is useful when computing the default value is + // expensive. + // // @@ Should we pass flags and interpret save_null_omitted to treat null // as undefined? Sounds logical. // @@ -135,7 +141,9 @@ namespace build2 // The second version in addition sets the new_value argument as described // above. Note, however, that if the save_default_commented flag is // specified, then the default value is never considered "new" since for - // such variables absence of a value means it is the default value. + // such variables absence of a value means it is the default value. This + // flag is normally used for dynamically adjusting (e.g., hinted) default + // values. // // If override is true and the variable doesn't come from this root scope // or from the command line (i.e., it is inherited from the amalgamation), diff --git a/libbuild2/scope.cxx b/libbuild2/scope.cxx index 51704fb..d18c4eb 100644 --- a/libbuild2/scope.cxx +++ b/libbuild2/scope.cxx @@ -175,11 +175,11 @@ namespace build2 return make_pair (lookup_type (), size_t (~0)); } - pair scope:: - lookup_override (const variable& var, - pair original, - bool target, - bool rule) const + auto scope:: + lookup_override_info (const variable& var, + const pair original, + bool target, + bool rule) const -> override_info { assert (!rule || target); // Rule-specific is target-specific. @@ -346,7 +346,7 @@ namespace build2 } if (!apply) - return original; + return override_info {original, orig.defined ()}; assert (inner_vars != nullptr); @@ -567,7 +567,9 @@ namespace build2 // Use the location of the innermost value that contributed as the // location of the result. // - return make_pair (lookup_type (&cv, &var, vars), depth); + return override_info { + make_pair (lookup_type (&cv, &var, vars), depth), + orig.defined () && stem == orig}; } value& scope:: diff --git a/libbuild2/scope.hxx b/libbuild2/scope.hxx index 97ec448..c49778f 100644 --- a/libbuild2/scope.hxx +++ b/libbuild2/scope.hxx @@ -151,10 +151,30 @@ namespace build2 size_t start_depth = 1) const; pair - lookup_override (const variable&, + lookup_override (const variable& var, pair original, bool target = false, - bool rule = false) const; + bool rule = false) const + { + return lookup_override_info (var, original, target, rule).lookup; + } + + // As above but also return an indication of whether the resulting value + // is/is based (e.g., via append/prepend overrides) on the original or an + // "outright" override. Note that it will always be false if there is no + // original. + // + struct override_info + { + pair lookup; + bool original; + }; + + override_info + lookup_override_info (const variable&, + pair original, + bool target = false, + bool rule = false) const; // Return a value suitable for assignment (or append if you only want to // append to the value from this scope). If the value does not exist in diff --git a/libbuild2/variable.hxx b/libbuild2/variable.hxx index 9b9880a..bf0aa00 100644 --- a/libbuild2/variable.hxx +++ b/libbuild2/variable.hxx @@ -1523,7 +1523,11 @@ namespace build2 // then typify the cached value. // pair - insert (context&, K, const lookup& stem, size_t version, const variable&); + insert (context&, + K, + const lookup& stem, + size_t base_version, + const variable&); private: struct entry_type @@ -1535,7 +1539,7 @@ namespace build2 // variable_map::value_data value; - size_t version = 0; // Version on which this value is based. + size_t base_version = 0; // Version on which this value is based. // Location of the stem as well as the version on which this cache // value is based. Used to track the location and value of the stem @@ -1548,11 +1552,11 @@ namespace build2 // entry_type () = default; entry_type (variable_map::value_data val, - size_t ver, + size_t bver, const variable_map* svars, size_t sver) : value (move (val)), - version (ver), + base_version (bver), stem_vars (svars), stem_version (sver) {} }; diff --git a/libbuild2/variable.txx b/libbuild2/variable.txx index 26f00ab..51176ae 100644 --- a/libbuild2/variable.txx +++ b/libbuild2/variable.txx @@ -766,7 +766,7 @@ namespace build2 insert (context& ctx, K k, const lookup& stem, - size_t ver, + size_t bver, const variable& var) { using value_data = variable_map::value_data; @@ -788,7 +788,7 @@ namespace build2 // Cache hit. // if (i != m_.end () && - i->second.version == ver && + i->second.base_version == bver && i->second.stem_vars == svars && i->second.stem_version == sver && (var.type == nullptr || i->second.value.type == var.type)) @@ -807,7 +807,7 @@ namespace build2 if (p.second) p = m_.emplace (move (k), - entry_type {value_data (nullptr), ver, svars, sver}); + entry_type {value_data (nullptr), bver, svars, sver}); entry_type& e (p.first->second); @@ -817,14 +817,14 @@ namespace build2 // e.value.version++; // New value. } - else if (e.version != ver || - e.stem_vars != svars || + else if (e.base_version != bver || + e.stem_vars != svars || e.stem_version != sver) { // Cache invalidation. // - assert (e.version <= ver); - e.version = ver; + assert (e.base_version <= bver); + e.base_version = bver; if (e.stem_vars != svars) e.stem_vars = svars; -- cgit v1.1