From e20a351013745e8d6c3a0a99bd40c172ed0ae8be Mon Sep 17 00:00:00 2001
From: Boris Kolpackov <boris@codesynthesis.com>
Date: Thu, 14 Mar 2019 17:20:02 +0200
Subject: Add support for multiple variable overrides

Now we can do:

$ b config.cxx.coptions=-O3 config.cxx.coptions=-O0

Or even:

$ b config.cxx.coptions=-O3 config.cxx.coptions+=-g
---
 build2/b.cxx                        | 29 +++----------
 build2/cc/module.cxx                |  4 +-
 build2/config/operation.cxx         | 18 +++-----
 build2/config/utility.cxx           |  2 +-
 build2/config/utility.txx           |  2 +-
 build2/context.cxx                  | 60 ++++++++++----------------
 build2/dump.cxx                     |  5 +--
 build2/parser.cxx                   |  2 +-
 build2/scope.cxx                    | 28 ++++++-------
 build2/scope.hxx                    |  2 +-
 build2/target.hxx                   |  4 +-
 build2/test/script/script.cxx       |  2 +-
 build2/variable.cxx                 | 20 ++++-----
 build2/variable.hxx                 | 84 +++++++++++++++++++++++++++++--------
 old-tests/variable/override/test.sh |  6 +--
 tests/variable/override/testscript  | 30 +++++++++++++
 16 files changed, 168 insertions(+), 130 deletions(-)

diff --git a/build2/b.cxx b/build2/b.cxx
index 789bbc2e..882b4697 100644
--- a/build2/b.cxx
+++ b/build2/b.cxx
@@ -1296,11 +1296,10 @@ main (int argc, char* argv[])
           // Enter project-wide (as opposed to global) variable overrides.
           //
           // The mildly tricky part here is to distinguish the situation where
-          // we are bootstrapping the same project multiple times (which is
-          // ok) vs overriding the same variable multiple times (which is not
-          // ok). The first override that we set cannot possibly end up in the
-          // second sitution so if it is already set, then it can only be the
-          // first case.
+          // we are bootstrapping the same project multiple times. The first
+          // override that we set cannot already exist (because the override
+          // variable names are unique) so if it is already set, then it can
+          // only mean this project is already bootstrapped.
           //
           // This is further complicated by the project vs amalgamation logic
           // (we may have already done the amalgamation but not the project).
@@ -1309,7 +1308,6 @@ main (int argc, char* argv[])
           {
             auto& sm (scope_map::instance);
 
-            bool first_a (true);
             for (const variable_override& o: var_ovs)
             {
               if (o.ovr.visibility != variable_visibility::normal)
@@ -1325,20 +1323,12 @@ main (int argc, char* argv[])
               auto p (s.vars.insert (o.ovr));
 
               if (!p.second)
-              {
-                if (first_a)
-                  break;
-
-                fail << "multiple " << (o.dir ? "scope" : "amalgamation")
-                     << " overrides of variable " << o.var.name;
-              }
+                break;
 
               value& v (p.first);
               v = o.val;
-              first_a = false;
             }
 
-            bool first_p (true);
             for (const variable_override& o: var_ovs)
             {
               // Ours is either project (%foo) or scope (/foo).
@@ -1353,17 +1343,10 @@ main (int argc, char* argv[])
               auto p (s.vars.insert (o.ovr));
 
               if (!p.second)
-              {
-                if (first_p)
-                  break;
-
-                fail << "multiple " << (o.dir ? "scope" : "project")
-                     << " overrides of variable " << o.var.name;
-              }
+                break;
 
               value& v (p.first);
               v = o.val;
-              first_p = false;
             }
           }
 
diff --git a/build2/cc/module.cxx b/build2/cc/module.cxx
index 78e2e0f9..824eee7f 100644
--- a/build2/cc/module.cxx
+++ b/build2/cc/module.cxx
@@ -191,7 +191,7 @@ namespace build2
 
       rs.assign (x_pattern) = ci.pattern;
 
-      if (!x_stdlib.aliases (c_stdlib))
+      if (!x_stdlib.alias (c_stdlib))
         rs.assign (x_stdlib) = ci.x_stdlib;
 
       new_ = p.second;
@@ -390,7 +390,7 @@ namespace build2
           dr << "\n  runtime    " << ci.runtime
              << "\n  stdlib     " << ci.x_stdlib;
 
-          if (!x_stdlib.aliases (c_stdlib))
+          if (!x_stdlib.alias (c_stdlib))
             dr << "\n  c stdlib   " << ci.c_stdlib;
         }
 
diff --git a/build2/config/operation.cxx b/build2/config/operation.cxx
index 724b92d4..7d6fc745 100644
--- a/build2/config/operation.cxx
+++ b/build2/config/operation.cxx
@@ -131,7 +131,7 @@ namespace build2
             const variable& var (sv.var);
 
             pair<lookup, size_t> org (root.find_original (var));
-            pair<lookup, size_t> ovr (var.override == nullptr
+            pair<lookup, size_t> ovr (var.overrides == nullptr
                                       ? org
                                       : root.find_override (var, org));
             const lookup& l (ovr.first);
@@ -859,19 +859,11 @@ namespace build2
       {
         const variable& var (p.first->first);
 
-        // Annoyingly, this is one of the __override/__prefix/__suffix
-        // values. So we strip the last component.
+        // Annoyingly, this can be (always is?) one of the overrides
+        // (__override, __prefix, etc).
         //
-        size_t n (var.name.size ());
-
-        if (var.name.compare (n - 11, 11, ".__override") == 0)
-          n -= 11;
-        else if (var.name.compare (n - 9, 9, ".__prefix") == 0)
-          n -= 9;
-        else if (var.name.compare (n - 9, 9, ".__suffix") == 0)
-          n -= 9;
-
-        m.save_variable (*vp.find (string (var.name, 0, n)));
+        size_t n (var.override ());
+        m.save_variable (n != 0 ? *vp.find (string (var.name, 0, n)) : var);
       }
 
       // Now project-specific. For now we just save all of them and let
diff --git a/build2/config/utility.cxx b/build2/config/utility.cxx
index cc89d2fa..e15c6898 100644
--- a/build2/config/utility.cxx
+++ b/build2/config/utility.cxx
@@ -33,7 +33,7 @@ namespace build2
       if (l.defined () && l->extra)
         n = true;
 
-      if (var.override != nullptr)
+      if (var.overrides != nullptr)
       {
         pair<lookup, size_t> ovr (r.find_override (var, move (org)));
 
diff --git a/build2/config/utility.txx b/build2/config/utility.txx
index 4f3429f2..b8e0acc2 100644
--- a/build2/config/utility.txx
+++ b/build2/config/utility.txx
@@ -47,7 +47,7 @@ namespace build2
       else if (l->extra)
         n = (save_flags & save_commented) == 0; // Absence means default.
 
-      if (var.override != nullptr)
+      if (var.overrides != nullptr)
       {
         pair<lookup, size_t> ovr (root.find_override (var, move (org)));
 
diff --git a/build2/context.cxx b/build2/context.cxx
index 633f3ff6..e1aaeee0 100644
--- a/build2/context.cxx
+++ b/build2/context.cxx
@@ -557,8 +557,10 @@ namespace build2
     // marked as such first. Then, as we enter variables, we can verify that
     // the override is alowed.
     //
-    for (const string& s: cmd_vars)
+    for (size_t i (0); i != cmd_vars.size (); ++i)
     {
+      const string& s (cmd_vars[i]);
+
       istringstream is (s);
       is.exceptions (istringstream::failbit | istringstream::badbit);
 
@@ -678,48 +680,39 @@ namespace build2
       if (c == '!' && dir)
         fail << "scope-qualified global override of variable " << n;
 
-      variable_visibility v (c == '/' ? variable_visibility::scope   :
-                             c == '%' ? variable_visibility::project :
-                             variable_visibility::normal);
-
       variable& var (const_cast<variable&> (
                        vp.insert (n, true /* overridable */)));
-      const char* k (tt == token_type::assign ? ".__override" :
-                     tt == token_type::append ? ".__suffix" : ".__prefix");
 
-      // We might already have a variable for this kind of override.
-      //
-      const variable* o (var.override.get ());
-      for (; o != nullptr; o = o->override.get ())
+      const variable* o;
       {
-        if (o->visibility == v && o->name.rfind (k) != string::npos)
-          break;
-      }
+        variable_visibility v (c == '/' ? variable_visibility::scope   :
+                               c == '%' ? variable_visibility::project :
+                               variable_visibility::normal);
+
+        const char* k (tt == token_type::assign ? "__override" :
+                       tt == token_type::append ? "__suffix" : "__prefix");
 
-      // Add it if not found.
-      //
-      if (o == nullptr)
-      {
         unique_ptr<variable> p (
           new variable {
-            n + k,
-            nullptr /* alias    */,
-            nullptr /* type     */,
-            nullptr /* override */,
+            n + '.' + to_string (i + 1) + '.' + k,
+            nullptr /* aliases   */,
+            nullptr /* type      */,
+            nullptr /* overrides */,
             v});
 
         // Back link.
         //
-        p->alias = p.get ();
-        if (var.override != nullptr)
-          swap (p->alias, const_cast<variable*> (var.override.get ())->alias);
+        p->aliases = p.get ();
+        if (var.overrides != nullptr)
+          swap (p->aliases,
+                const_cast<variable*> (var.overrides.get ())->aliases);
 
         // Forward link.
         //
-        p->override = move (var.override);
-        var.override = move (p);
+        p->overrides = move (var.overrides);
+        var.overrides = move (p);
 
-        o = var.override.get ();
+        o = var.overrides.get ();
       }
 
       // Currently we expand project overrides in the global scope to keep
@@ -745,16 +738,9 @@ namespace build2
       if (c == '!' || (dir && dir->absolute ()))
       {
         scope& s (c == '!' ? gs : sm.insert (*dir)->second);
-        auto p (s.vars.insert (*o));
 
-        if (!p.second)
-        {
-          if (c == '!')
-            fail << "multiple global overrides of variable " << n;
-          else
-            fail << "multiple overrides of variable " << n
-                 << " in scope " << *dir;
-        }
+        auto p (s.vars.insert (*o));
+        assert (p.second); // Variable name is unique.
 
         value& v (p.first);
         v = move (r.first);
diff --git a/build2/dump.cxx b/build2/dump.cxx
index 8f584889..d9a1855d 100644
--- a/build2/dump.cxx
+++ b/build2/dump.cxx
@@ -96,10 +96,7 @@ namespace build2
       //
       if (k != variable_kind::prerequisite)
       {
-        if (var.override != nullptr &&
-            var.name.rfind (".__override") == string::npos &&
-            var.name.rfind (".__suffix") == string::npos &&
-            var.name.rfind (".__prefix") == string::npos)
+        if (var.overrides != nullptr && !var.override ())
         {
           lookup org (v, var, vm);
 
diff --git a/build2/parser.cxx b/build2/parser.cxx
index 3acff378..997a1ac9 100644
--- a/build2/parser.cxx
+++ b/build2/parser.cxx
@@ -5108,7 +5108,7 @@ namespace build2
       if (!r.first.defined ())
         r = t->find_original (var);
 
-      return var.override == nullptr
+      return var.overrides == nullptr
         ? r.first
         : t->base_scope ().find_override (var, move (r), true).first;
     }
diff --git a/build2/scope.cxx b/build2/scope.cxx
index 4dd475dc..ec2776e3 100644
--- a/build2/scope.cxx
+++ b/build2/scope.cxx
@@ -191,7 +191,7 @@ namespace build2
     // no overrides apply, then we return the original value and not its copy
     // in the cache (this is used to detect if the value was overriden).
     //
-    assert (var.override != nullptr);
+    assert (var.overrides != nullptr);
 
     const lookup& orig (original.first);
     size_t orig_depth (original.second);
@@ -269,13 +269,13 @@ namespace build2
       return true;
     };
 
-    // Return the override value if present in scope s and (optionally) ends
-    // with the specified suffix.
+    // Return the override value if present in scope s and (optionally) of
+    // the specified kind (__override, __prefix, etc).
     //
     auto find = [&s, &var] (const variable* o,
-                            const char* sf = nullptr) -> lookup
+                            const char* k = nullptr) -> lookup
     {
-      if (sf != nullptr && o->name.rfind (sf) == string::npos)
+      if (k != nullptr && !o->override (k))
         return lookup ();
 
       // Note: using the original as storage variable.
@@ -317,9 +317,9 @@ namespace build2
         inner_proj = s->root_scope ();
       }
 
-      for (const variable* o (var.override.get ());
+      for (const variable* o (var.overrides.get ());
            o != nullptr;
-           o = o->override.get ())
+           o = o->overrides.get ())
       {
         if (inner_vars != nullptr && !applies (o, inner_vars, inner_proj))
           continue;
@@ -400,9 +400,9 @@ namespace build2
       // Note that the override list is in the reverse order of appearance and
       // so we will naturally see the most recent override first.
       //
-      for (const variable* o (var.override.get ());
+      for (const variable* o (var.overrides.get ());
            o != nullptr;
-           o = o->override.get ())
+           o = o->overrides.get ())
       {
         // If we haven't yet found anything, then any override will still be
         // "visible" even if it doesn't apply.
@@ -410,7 +410,7 @@ namespace build2
         if (stem.defined () && !applies (o, stem.vars, stem_proj))
           continue;
 
-        auto l (find (o, ".__override"));
+        auto l (find (o, "__override"));
 
         if (l.defined ())
         {
@@ -500,9 +500,9 @@ namespace build2
       //
       bool skip (stem_ovr != nullptr && stem_depth == ovr_depth);
 
-      for (const variable* o (var.override->alias); // Last override.
+      for (const variable* o (var.overrides->aliases); // Last override.
            o != nullptr;
-           o = (o->alias != var.override->alias ? o->alias : nullptr))
+           o = (o->aliases != var.overrides->aliases ? o->aliases : nullptr))
       {
         if (skip)
         {
@@ -524,8 +524,8 @@ namespace build2
         // variable itself is typed. We also pass the original variable for
         // diagnostics.
         //
-        auto lp (find (o, ".__prefix"));
-        auto ls (find (o, ".__suffix"));
+        auto lp (find (o, "__prefix"));
+        auto ls (find (o, "__suffix"));
 
         if (cl)
         {
diff --git a/build2/scope.hxx b/build2/scope.hxx
index 6e48de35..cfb3d245 100644
--- a/build2/scope.hxx
+++ b/build2/scope.hxx
@@ -125,7 +125,7 @@ namespace build2
           const string* tn = nullptr) const
     {
       auto p (find_original (var, tt, tn));
-      return var.override == nullptr ? p : find_override (var, move (p));
+      return var.overrides == nullptr ? p : find_override (var, move (p));
     }
 
     // Implementation details (used by scope target lookup). The start_depth
diff --git a/build2/target.hxx b/build2/target.hxx
index c86a10bf..fafb689a 100644
--- a/build2/target.hxx
+++ b/build2/target.hxx
@@ -399,7 +399,7 @@ namespace build2
     find (const variable& var) const
     {
       auto p (find_original (var));
-      return var.override == nullptr
+      return var.overrides == nullptr
         ? p
         : base_scope ().find_override (var, move (p), true);
     }
@@ -534,7 +534,7 @@ namespace build2
       find (const variable& var) const
       {
         auto p (find_original (var));
-        return var.override == nullptr
+        return var.overrides == nullptr
           ? p
           : target_->base_scope ().find_override (var, move (p), true, true);
       }
diff --git a/build2/test/script/script.cxx b/build2/test/script/script.cxx
index 94d6d8b8..865c7981 100644
--- a/build2/test/script/script.cxx
+++ b/build2/test/script/script.cxx
@@ -653,7 +653,7 @@ namespace build2
 
           if (p.first)
           {
-            if (var.override != nullptr)
+            if (var.overrides != nullptr)
               p = s.target_scope.find_override (var, move (p), true);
 
             return p.first;
diff --git a/build2/variable.cxx b/build2/variable.cxx
index 264928fe..173efe5d 100644
--- a/build2/variable.cxx
+++ b/build2/variable.cxx
@@ -1142,7 +1142,7 @@ namespace build2
     // Check overridability (all overrides, if any, should already have
     // been entered (see context.cxx:reset()).
     //
-    if (var.override != nullptr && (o == nullptr || !*o))
+    if (var.overrides != nullptr && (o == nullptr || !*o))
       fail << "variable " << var.name << " cannot be overridden";
 
     bool ut (t != nullptr && var.type != t);
@@ -1150,7 +1150,7 @@ namespace build2
 
     // Variable should not be updated post-aliasing.
     //
-    assert (var.alias == &var || (!ut && !uv));
+    assert (var.aliases == &var || (!ut && !uv));
 
     // Update type?
     //
@@ -1277,12 +1277,12 @@ namespace build2
     variable& r (p.first->second);
 
     if (p.second)
-      r.alias = &r;
+      r.aliases = &r;
     else // Note: overridden variable will always exist.
     {
       if (t != nullptr || v != nullptr || o != nullptr)
         update (r, t, v, o); // Not changing the key.
-      else if (r.override != nullptr)
+      else if (r.overrides != nullptr)
         fail << "variable " << r.name << " cannot be overridden";
     }
 
@@ -1292,7 +1292,7 @@ namespace build2
   const variable& variable_pool::
   insert_alias (const variable& var, string n)
   {
-    assert (var.alias != nullptr && var.override == nullptr);
+    assert (var.aliases != nullptr && var.overrides == nullptr);
 
     variable& a (insert (move (n),
                          var.type,
@@ -1300,13 +1300,13 @@ namespace build2
                          nullptr /* override */,
                          false   /* pattern  */));
 
-    if (a.alias == &a) // Not aliased yet.
+    if (a.aliases == &a) // Not aliased yet.
     {
-      a.alias = var.alias;
-      const_cast<variable&> (var).alias = &a;
+      a.aliases = var.aliases;
+      const_cast<variable&> (var).aliases = &a;
     }
     else
-      assert (a.aliases (var)); // Make sure it is already an alias of var.
+      assert (a.alias (var)); // Make sure it is already an alias of var.
 
     return a;
   }
@@ -1406,7 +1406,7 @@ namespace build2
         break;
       }
 
-      v = v->alias;
+      v = v->aliases;
 
     } while (v != &var && v != nullptr);
 
diff --git a/build2/variable.hxx b/build2/variable.hxx
index 2bdfe30d..5e6ad2fc 100644
--- a/build2/variable.hxx
+++ b/build2/variable.hxx
@@ -145,25 +145,39 @@ namespace build2
   // The two variables are considered the same if they have the same name.
   //
   // Variables can be aliases of each other in which case they form a circular
-  // linked list (alias for variable without any aliases points to the
-  // variable itself).
+  // linked list (the aliases pointer for variable without any aliases points
+  // to the variable itself).
   //
   // If the variable is overridden on the command line, then override is the
   // linked list of the special override variables. Their names are derived
-  // from the main variable name as <name>.{__override,__prefix,__suffix} and
-  // they are not entered into the var_pool. The override variables only vary
-  // in their names and visibility. Their alias pointer is re-purposed to make
-  // the list doubly-linked with the first override's alias pointing to the
-  // last element (or itself).
-  //
-  // Note that the override list is in the reverse order of the overrides
-  // appearing on the command line, which is important when deciding whether
-  // and in what order they apply (see find_override() for details).
+  // from the main variable name as <name>.<N>.{__override,__prefix,__suffix}
+  // and they are not entered into the var_pool. The override variables only
+  // vary in their names and visibility. Their aliases pointer is re-purposed
+  // to make the list doubly-linked with the first override's aliases pointer
+  // pointing to the last element (or itself).
   //
   // Note also that we don't propagate the variable type to override variables
   // and we keep override values as untyped names. They get "typed" when they
   // are applied.
   //
+  // The overrides list is in the reverse order of the overrides appearing on
+  // the command line, which is important when deciding whether and in what
+  // order they apply (see find_override() for details).
+  //
+  // The <N> part in the override variable name is its position on the command
+  // line, which effectively means we will have as many variable names as
+  // there are overrides. This strange arrangement is here to support multiple
+  // overrides. For example:
+  //
+  // b config.cc.coptions=-O2 config.cc.coptions+=-g config.cc.coptions+=-Wall
+  //
+  // We cannot yet apply them to form a single value since this requires
+  // knowing their type. And there is no way to store multiple values of the
+  // same variable in any given variable_map. As a result, the best option
+  // appears to be to store them as multiple variables. While not very
+  // efficient, this shouldn't be a big deal since we don't expect to have
+  // many overrides.
+  //
   // We use the "modify original, override on query" model. Because of that, a
   // modified value does not necessarily represent the actual value so care
   // must be taken to re-query after (direct) modification. And because of
@@ -179,20 +193,56 @@ namespace build2
   struct variable
   {
     string name;
-    const variable* alias;               // Circular linked list.
-    const value_type* type;              // If NULL, then not (yet) typed.
-    unique_ptr<const variable> override;
+    const variable* aliases;               // Circular linked list.
+    const value_type* type;                // If NULL, then not (yet) typed.
+    unique_ptr<const variable> overrides;
     variable_visibility visibility;
 
     // Return true if this variable is an alias of the specified variable.
     //
     bool
-    aliases (const variable& var) const
+    alias (const variable& var) const
     {
-      const variable* v (alias);
-      for (; v != &var && v != this; v = v->alias) ;
+      const variable* v (aliases);
+      for (; v != &var && v != this; v = v->aliases) ;
       return v == &var;
     }
+
+    // Return the length of the original variable if this is an override,
+    // optionally of the specified kind (__override, __prefix, etc), and 0
+    // otherwise (so this function can be used as a predicate).
+    //
+    // @@ It would be nicer to return the original variable but there is no
+    //    natural place to store such a "back" pointer. The overrides pointer
+    //    in the last element could work but it is owning. So let's not
+    //    complicate things for now seeing that there are only a few places
+    //    where we need this.
+    //
+    size_t
+    override (const char* k = nullptr) const
+    {
+      size_t p (name.rfind ('.'));
+      if (p != string::npos)
+      {
+        auto cmp = [this, p] (const char* k)
+        {
+          return name.compare (p + 1, string::npos, k) == 0;
+        };
+
+        if (k != nullptr
+            ? (cmp (k))
+            : (cmp ("__override") || cmp ("__prefix") || cmp ("__suffix")))
+        {
+          // Skip .<N>.
+          //
+          p = name.rfind ('.', p - 1);
+          assert (p != string::npos && p != 0);
+          return p;
+        }
+      }
+
+      return 0;
+    }
   };
 
   inline bool
diff --git a/old-tests/variable/override/test.sh b/old-tests/variable/override/test.sh
index baef1ca3..94ed61fe 100755
--- a/old-tests/variable/override/test.sh
+++ b/old-tests/variable/override/test.sh
@@ -55,9 +55,9 @@ function test ()
 
 fail foo=bar[]             # error: unexpected [ in variable assignment 'foo=bar[]'
 fail foo=[string]bar       # error: typed override of variable foo
-fail "!foo=bar" "!foo=BAR" # error: multiple global overrides of variable foo
-fail "foo=bar" "foo=BAR"   # error: multiple project overrides of variable foo
-fail "%foo=bar" "%foo=BAR" # error: multiple project overrides of variable foo
+#fail "!foo=bar" "!foo=BAR" # error: multiple global overrides of variable foo
+#fail "foo=bar" "foo=BAR"   # error: multiple project overrides of variable foo
+#fail "%foo=bar" "%foo=BAR" # error: multiple project overrides of variable foo
 
 test --buildfile simple foo=bar ./ ./ <<< "bar"  # Multiple bootstraps of the same project.
 
diff --git a/tests/variable/override/testscript b/tests/variable/override/testscript
index 28225c1a..f8a930b7 100644
--- a/tests/variable/override/testscript
+++ b/tests/variable/override/testscript
@@ -105,3 +105,33 @@
   1 2
   EOO
 }
+
+: multiple
+:
+{
+  : assign
+  :
+  $* x=0 !y=0 x=1 !y=1 <<EOI >>EOO
+  print $x
+  print $y
+  EOI
+  1
+  1
+  EOO
+
+  : append
+  :
+  $* x=0 x+=1 x+=2 <<EOI >>EOO
+  print $x
+  EOI
+  0 1 2
+  EOO
+
+  : prepend
+  :
+  $* x=2 x=+1 x=+0 <<EOI >>EOO
+  print $x
+  EOI
+  0 1 2
+  EOO
+}
-- 
cgit