Redo config inheritance logic

9 files changed, 118 insertions, 27 deletions

diff --git a/build2/config/operation.cxx b/build2/config/operation.cxx
index d1a01ea..fc583b5 100644
--- a/build2/config/operation.cxx
+++ b/build2/config/operation.cxx
@@ -102,16 +102,83 @@ namespace build2
         {
           const variable& var (p.first);
 
-          lookup l (root[var]);
-
-          // We only write values that are set on our root scope or are global
-          // overrides. Anything in-between is inherited. We might also not
-          // have any value at all (see unconfigured()).
+          pair<lookup, size_t> org (root.find_original (var));
+          pair<lookup, size_t> ovr (var.override == nullptr
+                                    ? org
+                                    : root.find_override (var, org));
+          const lookup& l (ovr.first);
+
+          // We definitely write values that are set on our root scope or are
+          // global overrides. Anything in-between is presumably inherited.
+          // We might also not have any value at all (see unconfigured()).
           //
-          if (!l.defined () ||
-              !(l.belongs (root) || l.belongs (*global_scope)))
+          if (!l.defined ())
             continue;
 
+          if (!(l.belongs (root) || l.belongs (*global_scope)))
+          {
+            // This is presumably an inherited value. But it could also be
+            // some left-over garbage. For example, our amalgamation could
+            // have used a module but then dropped it while its configuration
+            // values are still lingering in config.build. They are probably
+            // still valid and we should probably continue using them but we
+            // definitely want to move them to our config.build since they
+            // will be dropped from the amalgamation's config.build. Let's
+            // also warn the user just in case.
+            //
+            bool found (false);
+            scope* r (&root);
+            while ((r = r->parent_scope ()->root_scope ()) != nullptr)
+            {
+              if (l.belongs (*r))
+              {
+                if (auto* m = r->modules.lookup<const module> (module::name))
+                  found = m->vars.find (var) != m->vars.end ();
+
+                break;
+              }
+            }
+
+            if (found) // Inherited.
+              continue;
+
+            location loc (&f);
+
+            // If this value is not defined in a project's root scope, then
+            // something is broken.
+            //
+            if (r == nullptr)
+              fail (loc) << "inherited variable " << var.name << " value "
+                         << "is not from a root scope";
+
+            // If none of the outer project's configurations use this value,
+            // then we warn and save as our own. One special case where we
+            // don't want to warn the user is if the variable is overriden.
+            //
+            if (org.first == ovr.first)
+            {
+              diag_record dr;
+              dr << warn (loc) << "saving previously inherited variable "
+                 << var.name;
+
+              dr << info (loc) << "because project " << r->out_path ()
+                 << " no longer uses it in its configuration";
+
+              if (verb >= 2)
+              {
+                dr << info (loc) << "variable value: ";
+
+                if (*l)
+                {
+                  names storage;
+                  dr << "'" << reverse (*l, storage) << "'";
+                }
+                else
+                  dr << "[null]";
+              }
+            }
+          }
+
           const value& val (*l);
 
           // We will only write config.*.configured if it is false (true is
diff --git a/build2/config/utility b/build2/config/utility
index 451df30..8a93c09 100644
--- a/build2/config/utility
+++ b/build2/config/utility
@@ -21,12 +21,13 @@ namespace build2
     //
     // 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 amalgamtion),
-    // then its value is "overridden" for this root scope.
+    // then its value is "overridden" to the default value on this root scope.
     //
     // Return the reference to the value as well as the indication of whether
-    // the value is "new", that is, it was either set (including override) or
-    // it came from the command line and was not inherited. This is usually
-    // used to test the new value.
+    // the value is "new", that is, it was set to the default value (inherited
+    // or not, including overrides). We also treat command line overrides
+    // (inherited or not) as new. This flag is usually used to test that the
+    // new value is valid, etc.
     //
     template <typename T>
     pair<reference_wrapper<const value>, bool>
diff --git a/build2/config/utility.txx b/build2/config/utility.txx
index f49be75..30ed02f 100644
--- a/build2/config/utility.txx
+++ b/build2/config/utility.txx
@@ -17,8 +17,9 @@ namespace build2
         save_variable (root, var);
 
       pair<lookup, size_t> org (root.find_original (var));
+
+      bool n (false); // New flag.
       lookup l (org.first);
-      bool n (false);
 
       // The interaction with command line overrides can get tricky. For
       // example, the override to defaul value could make (non-recursive)
@@ -28,10 +29,17 @@ namespace build2
       //
       if (!l.defined () || (def_ovr && !l.belongs (root)))
       {
-        l = lookup ((root.assign (var) = def_val), root);
-        org = make_pair (l, 1); // Lookup depth is 1 since in root.vars.
+        value& v (root.assign (var) = def_val);
+        v.extra = true; // Default value flag.
+
         n = true;
+        l = lookup (v, root);
+        org = make_pair (l, 1); // Lookup depth is 1 since it's in root.vars.
       }
+      // Treat an inherited value that was set to default as new.
+      //
+      else if (l->extra)
+        n = true;
 
       if (var.override != nullptr)
       {
@@ -39,11 +47,10 @@ namespace build2
 
         if (l != ovr.first) // Overriden?
         {
-          l = move (ovr.first);
-
-          // Overriden and not inherited (same logic as in save_config()).
+          // Override is always treated as new.
           //
-          n = l.belongs (root) || l.belongs (*global_scope);
+          n = true;
+          l = move (ovr.first);
         }
       }
 
diff --git a/build2/diagnostics b/build2/diagnostics
index ffb24ff..c0ba6ec 100644
--- a/build2/diagnostics
+++ b/build2/diagnostics
@@ -280,10 +280,11 @@ namespace build2
   class location
   {
   public:
-    // Note that location maintains a shallow reference to path.
+    // Note that location maintains a shallow reference to path. Zero lines
+    // or columns are not printed.
     //
     location (): file (nullptr), line (0), column (0) {}
-    location (const path* f, uint64_t l, uint64_t c)
+    location (const path* f, uint64_t l = 0, uint64_t c = 0)
         : file (f), line (l), column (c) {}
 
     const path* file;
diff --git a/build2/diagnostics.cxx b/build2/diagnostics.cxx
index d6278a8..7b60d05 100644
--- a/build2/diagnostics.cxx
+++ b/build2/diagnostics.cxx
@@ -106,7 +106,15 @@ namespace build2
   {
     stream_verb (r.os_, sverb_);
 
-    r << *loc_.file << ':' << loc_.line << ':' << loc_.column << ": ";
+    r << *loc_.file << ':';
+
+    if (loc_.line != 0)
+      r << loc_.line << ':';
+
+    if (loc_.column != 0)
+      r << loc_.column << ':';
+
+    r << ' ';
 
     if (type_ != nullptr)
       r << type_ << ": ";
diff --git a/build2/scope b/build2/scope
index 09914a9..c1dcb91 100644
--- a/build2/scope
+++ b/build2/scope
@@ -146,7 +146,7 @@ namespace build2
 
     pair<lookup, size_t>
     find_override (const variable&,
-                   pair<lookup, size_t>&& original,
+                   pair<lookup, size_t> original,
                    bool target = false) const;
 
     // Return a value suitable for assignment (or append if you only
diff --git a/build2/scope.cxx b/build2/scope.cxx
index 19f3b56..f0d28e0 100644
--- a/build2/scope.cxx
+++ b/build2/scope.cxx
@@ -67,7 +67,7 @@ namespace build2
 
   pair<lookup, size_t> scope::
   find_override (const variable& var,
-                 pair<lookup, size_t>&& original,
+                 pair<lookup, size_t> original,
                  bool target) const
   {
     // Normally there would be no overrides and if there are, there will only
diff --git a/build2/variable b/build2/variable
index ebf0f02..ea60832 100644
--- a/build2/variable
+++ b/build2/variable
@@ -119,7 +119,7 @@ namespace build2
 
   // value
   //
-  enum class value_state {null, empty, filled};
+  enum class value_state: uint8_t {null, empty, filled};
 
   class value
   {
@@ -127,6 +127,13 @@ namespace build2
     const value_type* type; // NULL means this value is not (yet) typed.
     value_state state;
 
+    // Extra data that is associated with the value that can be used to store
+    // flags, etc. It is initialized to 0 and copied (but not assigned) from
+    // one value to another but is otherwise untouched (not even when the
+    // value is reset to NULL).
+    //
+    uint16_t extra;
+
     bool null () const {return state == value_state::null;}
     bool empty () const {return state == value_state::empty;}
 
@@ -145,7 +152,7 @@ namespace build2
 
     explicit
     value (const value_type* t = nullptr)
-        : type (t), state (value_state::null) {}
+        : type (t), state (value_state::null), extra (0) {}
 
     // Note: preserves type.
     //
diff --git a/build2/variable.cxx b/build2/variable.cxx
index c4e2721..d6a42b3 100644
--- a/build2/variable.cxx
+++ b/build2/variable.cxx
@@ -27,7 +27,7 @@ namespace build2
 
   value::
   value (value&& v)
-      : type (v.type), state (v.state)
+      : type (v.type), state (v.state), extra (v.extra)
   {
     if (!null ())
     {
@@ -42,7 +42,7 @@ namespace build2
 
   value::
   value (const value& v)
-      : type (v.type), state (v.state)
+      : type (v.type), state (v.state), extra (v.extra)
   {
     if (!null ())
     {