Cleanup variable type/visibility/overridability logic

3 files changed, 89 insertions, 65 deletions

diff --git a/libbuild2/parser.cxx b/libbuild2/parser.cxx
index e219e07..91f2d5c 100644
--- a/libbuild2/parser.cxx
+++ b/libbuild2/parser.cxx
@@ -2732,13 +2732,18 @@ namespace build2
   const variable& parser::
   parse_variable_name (names&& ns, const location& l)
   {
+    // Parse and enter a variable name for assignment (as opposed to lookup).
+
     // The list should contain a single, simple name.
     //
     if (ns.size () != 1 || !ns[0].simple () || ns[0].empty ())
       fail (l) << "expected variable name instead of " << ns;
 
-    return scope_->var_pool ().insert (move (ns[0].value),
-                                       true /* overridable */);
+    // Note that the overridability can still be restricted (e.g., by a module
+    // that enters this variable or by a pattern).
+    //
+    return scope_->var_pool ().insert (
+      move (ns[0].value), true /* overridable */);
   }
 
   void parser::
@@ -2902,7 +2907,10 @@ namespace build2
       return;
 
     const location& l (a.loc);
+
     const value_type* type (nullptr);
+    optional<variable_visibility> vis;
+    optional<bool> ovr;
 
     auto print = [storage = names ()] (diag_record& dr, const value& v) mutable
     {
@@ -2943,17 +2951,23 @@ namespace build2
       }
     }
 
-    if (type != nullptr)
+    if (type != nullptr && var.type != nullptr)
     {
-      if (var.type == nullptr)
-      {
-        const bool o (true); // Allow overrides.
-        ctx.var_pool.update (const_cast<variable&> (var), type, nullptr, &o);
-      }
-      else if (var.type != type)
+      if (var.type == type)
+        type = nullptr;
+      else
         fail (l) << "changing variable " << var << " type from "
                  << var.type->name << " to " << type->name;
     }
+
+    //@@ TODO: the same for vis and ovr.
+
+    if (type || vis || ovr)
+      ctx.var_pool.update (const_cast<variable&> (var),
+                           type,
+                           vis ? &*vis : nullptr,
+                           ovr ? &*ovr : nullptr);
+
   }
 
   void parser::
@@ -5720,46 +5734,46 @@ namespace build2
 
     // Lookup.
     //
-    // @@ Why insert instead of find(), like in []-lookup? Also change to
-    //    const string& name.
-    //
-    const variable& var (scope_->var_pool ().insert (move (name), true));
-
-    if (p != nullptr)
+    if (const variable* pvar = scope_->var_pool ().find (name))
     {
-      // The lookup depth is a bit of a hack but should be harmless since
-      // unused.
-      //
-      pair<lookup, size_t> r (p->vars[var], 1);
+      auto& var (*pvar);
+
+      if (p != nullptr)
+      {
+        // The lookup depth is a bit of a hack but should be harmless since
+        // unused.
+        //
+        pair<lookup, size_t> r (p->vars[var], 1);
 
-      if (!r.first.defined ())
-        r = t->lookup_original (var);
+        if (!r.first.defined ())
+          r = t->lookup_original (var);
 
-      return var.overrides == nullptr
-        ? r.first
-        : t->base_scope ().lookup_override (var, move (r), true).first;
-    }
+        return var.overrides == nullptr
+          ? r.first
+          : t->base_scope ().lookup_override (var, move (r), true).first;
+      }
 
-    if (t != nullptr)
-    {
-      if (var.visibility > variable_visibility::target)
+      if (t != nullptr)
       {
-        fail (loc) << "variable " << var << " has " << var.visibility
-                   << " visibility but is expanded in target context";
-      }
+        if (var.visibility > variable_visibility::target)
+        {
+          fail (loc) << "variable " << var << " has " << var.visibility
+                     << " visibility but is expanded in target context";
+        }
 
-      return (*t)[var];
-    }
+        return (*t)[var];
+      }
 
-    if (s != nullptr)
-    {
-      if (var.visibility > variable_visibility::scope)
+      if (s != nullptr)
       {
-        fail (loc) << "variable " << var << " has " << var.visibility
-                   << " visibility but is expanded in scope context";
-      }
+        if (var.visibility > variable_visibility::scope)
+        {
+          fail (loc) << "variable " << var << " has " << var.visibility
+                     << " visibility but is expanded in scope context";
+        }
 
-      return (*s)[var];
+        return (*s)[var];
+      }
     }
 
     return lookup ();
diff --git a/libbuild2/variable.cxx b/libbuild2/variable.cxx
index f1c5515..48fcf40 100644
--- a/libbuild2/variable.cxx
+++ b/libbuild2/variable.cxx
@@ -1143,7 +1143,7 @@ namespace build2
     // Check overridability (all overrides, if any, should already have
     // been entered; see context ctor for details).
     //
-    if (var.overrides != nullptr && (o == nullptr || !*o))
+    if (o != nullptr && var.overrides != nullptr && !*o)
       fail << "variable " << var.name << " cannot be overridden";
 
     bool ut (t != nullptr && var.type != t);
@@ -1285,25 +1285,23 @@ namespace build2
       var.aliases = &var;
     else // Note: overridden variable will always exist.
     {
-      if (pt != nullptr || pv != nullptr || po != nullptr)
+      // This is tricky: if the pattern does not require a match, then we
+      // should re-merge it with values that came from the variable.
+      //
+      bool vo;
+      if (pa != nullptr && !pa->match)
       {
-        // This is tricky: if the pattern does not require a match, then we
-        // should re-merge it with values that came from the variable.
-        //
-        bool vo;
-        if (pa != nullptr && !pa->match)
-        {
-          pt = t != nullptr ? t : var.type;
-          pv = v != nullptr ? v : &var.visibility;
-          po = o != nullptr ? o : &(vo = (var.overrides != nullptr));
-
-          merge_pattern (*pa, pt, pv, po);
-        }
+        pt = t != nullptr ? t : var.type;
+        pv = v != nullptr ? v : &var.visibility;
+        po = o != nullptr ? o : &(vo = true);
 
-        update (var, pt, pv, po); // Not changing the key.
+        merge_pattern (*pa, pt, pv, po);
       }
-      else if (var.overrides != nullptr)
-        fail << "variable " << var << " cannot be overridden";
+
+      if (po == nullptr) // NULL overridable falls back to false.
+        po = &(vo = false);
+
+      update (var, pt, pv, po); // Not changing the key.
     }
 
     return var;
@@ -1320,6 +1318,8 @@ namespace build2
                          nullptr /* override */,
                          false   /* pattern  */));
 
+    assert (a.overrides == nullptr);
+
     if (a.aliases == &a) // Not aliased yet.
     {
       a.aliases = var.aliases;
diff --git a/libbuild2/variable.hxx b/libbuild2/variable.hxx
index b293215..9b9880a 100644
--- a/libbuild2/variable.hxx
+++ b/libbuild2/variable.hxx
@@ -1065,9 +1065,12 @@ namespace build2
     const variable*
     find (const string& name) const;
 
-    // Find existing or insert new (untyped, non-overridable, normal
-    // visibility; but may be overridden by a pattern).
+    // Find existing or insert new variable.
     //
+    // Unless specified explicitly, the variable is untyped, non-overridable,
+    // and with normal visibility but these may be overridden by a pattern.
+    // Note also that a pattern may restrict (but not relax) overridability.
+
     const variable&
     insert (string name)
     {
@@ -1083,6 +1086,9 @@ namespace build2
       return insert (move (name), nullptr, &v, nullptr);
     }
 
+    // Note that overridability can still be restricted (but not relaxed) by
+    // another call to insert or via patterns (see below).
+    //
     const variable&
     insert (string name, bool overridable)
     {
@@ -1099,14 +1105,15 @@ namespace build2
     const variable&
     insert (string name)
     {
-      return insert (move (name), &value_traits<T>::value_type);
+      return insert (
+        move (name), &value_traits<T>::value_type, nullptr, nullptr);
     }
 
     template <typename T>
     const variable&
     insert (string name, variable_visibility v)
     {
-      return insert (move (name), &value_traits<T>::value_type, &v);
+      return insert (move (name), &value_traits<T>::value_type, &v, nullptr);
     }
 
     template <typename T>
@@ -1213,18 +1220,21 @@ namespace build2
     friend class scope;
 
   private:
+    // Note that in insert() NULL overridable is interpreted as false unless
+    // overridden by a pattern while in update() NULL overridable is ignored.
+    //
     LIBBUILD2_SYMEXPORT variable&
     insert (string name,
             const value_type*,
-            const variable_visibility* = nullptr,
-            const bool* overridable = nullptr,
+            const variable_visibility*,
+            const bool* overridable,
             bool pattern = true);
 
     LIBBUILD2_SYMEXPORT void
     update (variable&,
             const value_type*,
-            const variable_visibility* = nullptr,
-            const bool* = nullptr) const;
+            const variable_visibility*,
+            const bool*) const;
 
     // Variable map.
     //