Tweak lookup_config() semantics some more

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<lookup, size_t> 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<lookup, size_t>& 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<lookup, size_t> scope::
-  lookup_override (const variable& var,
-                   pair<lookup_type, size_t> original,
-                   bool target,
-                   bool rule) const
+  auto scope::
+  lookup_override_info (const variable& var,
+                        const pair<lookup_type, size_t> 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_type, size_t>
-    lookup_override (const variable&,
+    lookup_override (const variable& var,
                      pair<lookup_type, size_t> 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_type, size_t> lookup;
+      bool original;
+    };
+
+    override_info
+    lookup_override_info (const variable&,
+                          pair<lookup_type, size_t> 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<value&, ulock>
-    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;