Clean up variable lookup interfaces

15 files changed, 121 insertions, 167 deletions

diff --git a/build2/cxx/compile.cxx b/build2/cxx/compile.cxx
index c4e9cee..914cd10 100644
--- a/build2/cxx/compile.cxx
+++ b/build2/cxx/compile.cxx
@@ -236,7 +236,7 @@ namespace build2
       auto test = [&s, &n, &e, &var] (const target_type& tt)
         -> const target_type*
       {
-        if (auto l = s.lookup (tt, n, var))
+        if (auto l = s.find (var, tt, n))
           if (cast<string> (l) == e)
             return &tt;
 
diff --git a/build2/cxx/link.cxx b/build2/cxx/link.cxx
index aa60c7e..e4154ab 100644
--- a/build2/cxx/link.cxx
+++ b/build2/cxx/link.cxx
@@ -780,7 +780,7 @@ namespace build2
       if (dd.expect ("cxx.link 1") != nullptr)
         l4 ([&]{trace << "rule mismatch forcing update of " << t;});
 
-      lookup<const value> ranlib;
+      lookup ranlib;
 
       // Then the linker checksum (ar/ranlib or C++ compiler).
       //
@@ -789,7 +789,7 @@ namespace build2
         ranlib = rs["config.bin.ranlib"];
 
         if (ranlib->empty ()) // @@ TMP until proper NULL support.
-          ranlib = lookup<const value> ();
+          ranlib = lookup ();
 
         const char* rl (
           ranlib
diff --git a/build2/dist/operation.cxx b/build2/dist/operation.cxx
index 86917ed..8a890fa 100644
--- a/build2/dist/operation.cxx
+++ b/build2/dist/operation.cxx
@@ -72,7 +72,7 @@ namespace build2
       // Make sure we have the necessary configuration before
       // we get down to business.
       //
-      auto l (rs->vars["dist.root"]);
+      auto l (rs->vars["dist.root"]); //@@ OVR
 
       if (!l || l->empty ())
         fail << "unknown root distribution directory" <<
@@ -84,14 +84,14 @@ namespace build2
         fail << "root distribution directory " << dist_root
              << " does not exist";
 
-      l = rs->vars["dist.package"];
+      l = rs->vars["dist.package"]; //@@ OVR
 
       if (!l || l->empty ())
         fail << "unknown distribution package name" <<
           info << "did you forget to set dist.package?";
 
       const string& dist_package (cast<string> (l));
-      const path& dist_cmd (cast<path> (rs->vars["dist.cmd"]));
+      const path& dist_cmd (cast<path> (rs->vars["dist.cmd"])); // @@ OVR
 
       // Get the list of operations supported by this project. Skip
       // default_id.
@@ -281,7 +281,7 @@ namespace build2
 
       // Archive if requested.
       //
-      if (auto l = rs->vars["dist.archives"])
+      if (auto l = rs->vars["dist.archives"]) // @@ OVR
       {
         for (const string& e: cast<strings> (l))
           archive (dist_root, dist_package, e);
diff --git a/build2/file.cxx b/build2/file.cxx
index 1f4e517..10461c4 100644
--- a/build2/file.cxx
+++ b/build2/file.cxx
@@ -263,9 +263,9 @@ namespace build2
     source_once (bf, root, root);
   }
 
-  // Extract the specified variable value from a buildfile. It is
-  // expected to be the first non-comment line and not to rely on
-  // any variable expansion other than those from the global scope.
+  // Extract the specified variable value from a buildfile. It is expected to
+  // be the first non-comment line and not to rely on any variable expansion
+  // other than those from the global scope or any variable overrides.
   //
   static value
   extract_variable (const path& bf, const char* name)
@@ -297,10 +297,9 @@ namespace build2
       temp_scope tmp (*global_scope);
       p.parse_variable (lex, tmp, var, tt);
 
-      auto l (tmp.vars[var]);
-      assert (l.defined ());
-      value& v (*l);
-      return move (v); // Steal the value, the scope is going away.
+      value* v (tmp.vars.find (var));
+      assert (v != nullptr);
+      return move (*v); // Steal the value, the scope is going away.
     }
     catch (const ifstream::failure&)
     {
diff --git a/build2/scope b/build2/scope
index 7ec8c3b..6050bf8 100644
--- a/build2/scope
+++ b/build2/scope
@@ -85,51 +85,52 @@ namespace build2
   public:
     variable_map vars;
 
-    // Lookup, including in outer scopes. If you only want to lookup
-    // in this scope, do it on the the variables map directly.
+    // Lookup, including in outer scopes. If you only want to lookup in this
+    // scope, do it on the the variables map directly (and note that there
+    // will be no overrides).
     //
-    build2::lookup<const value>
+    lookup
     operator[] (const variable& var) const
     {
-      return lookup (nullptr, nullptr, var);
+      return find (var, nullptr, nullptr);
     }
 
-    build2::lookup<const value>
+    lookup
     operator[] (const string& name) const
     {
       return operator[] (var_pool.find (name));
     }
 
-    // As above, but includes target type/pattern-specific variables.
+    // As above, but include target type/pattern-specific variables.
     //
-    build2::lookup<const value>
-    lookup (const target_key& tk, const variable& var) const
+    lookup
+    find (const variable& var, const target_key& tk) const
     {
-      return lookup (tk.type, tk.name, var);
+      return find (var, tk.type, tk.name);
     }
 
-    build2::lookup<const value>
-    lookup (const target_key& tk, const string& var) const
+    lookup
+    find (const string& var, const target_key& tk) const
     {
-      return lookup (tk, var_pool.find (var));
+      return find (var_pool.find (var), tk);
     }
 
-    build2::lookup<const value>
-    lookup (const target_type& tt,
-            const string& name,
-            const variable& var) const
+    lookup
+    find (const variable& var, const target_type& tt, const string& tn) const
     {
-      return lookup (&tt, &name, var);
+      return find (var, &tt, &tn);
     }
 
-    build2::lookup<const value>
-    lookup (const target_type& tt, const string& name, const string& var) const
+    lookup
+    find (const string& var, const target_type& tt, const string& tn) const
     {
-      return lookup (tt, name, var_pool.find (var));
+      return find (var_pool.find (var), tt, tn);
     }
 
-    build2::lookup<const value>
-    lookup (const target_type*, const string* name, const variable&) const;
+    lookup
+    find (const variable&,
+          const target_type* tt, const string* tn,
+          const target_type* gt = nullptr, const string* gn = nullptr) const;
 
     // Return a value suitable for assignment (or append if you only
     // want to append to the value from this scope). If the variable
diff --git a/build2/scope.cxx b/build2/scope.cxx
index 294833e..77efccd 100644
--- a/build2/scope.cxx
+++ b/build2/scope.cxx
@@ -12,21 +12,30 @@ namespace build2
 {
   // scope
   //
-  lookup<const value> scope::
-  lookup (const target_type* tt, const string* name, const variable& var) const
+  lookup scope::
+  find (const variable& var,
+        const target_type* tt, const string* tn,
+        const target_type* gt, const string* gn) const
   {
-    using result = build2::lookup<const value>;
-
     for (const scope* s (this); s != nullptr; )
     {
-      if (tt != nullptr && !s->target_vars.empty ())
+      if (!s->target_vars.empty ())
       {
-        if (auto l = s->target_vars.lookup (*tt, *name, var))
-          return l;
+        if (tt != nullptr)
+        {
+          if (auto l = s->target_vars.find (*tt, *tn, var))
+            return l;
+        }
+
+        if (gt != nullptr)
+        {
+          if (auto l = s->target_vars.find (*gt, *gn, var))
+            return l;
+        }
       }
 
       if (auto r = s->vars.find (var))
-        return result (r, &s->vars);
+        return lookup (r, &s->vars);
 
       switch (var.visibility)
       {
@@ -42,7 +51,7 @@ namespace build2
       }
     }
 
-    return result ();
+    return lookup ();
   }
 
   value& scope::
diff --git a/build2/target b/build2/target
index bf5ff04..af573fa 100644
--- a/build2/target
+++ b/build2/target
@@ -142,10 +142,9 @@ namespace build2
     virtual void
     reset (action_type);
 
-    const dir_path dir;      // Absolute and normalized.
+    const dir_path dir; // Absolute and normalized.
     const string name;
-    const string* ext;  // Extension, NULL means unspecified,
-                             // empty means no extension.
+    const string* ext;  // Extension, NULL - unspecified, empty - no extension.
 
     // Target group to which this target belongs, if any. Note that
     // we assume that the group and all its members are in the same
@@ -271,15 +270,15 @@ namespace build2
   public:
     variable_map vars;
 
-    // Lookup, including in groups to which this target belongs and
-    // then in outer scopes (including target type/pattern-specific
-    // variables). If you only want to lookup in this target, do it
-    // on the variable map directly.
+    // Lookup, including in groups to which this target belongs and then in
+    // outer scopes (including target type/pattern-specific variables). If you
+    // only want to lookup in this target, do it on the variable map directly
+    // (and note that there will be no overrides).
     //
-    lookup<const value>
+    lookup
     operator[] (const variable&) const;
 
-    lookup<const value>
+    lookup
     operator[] (const string& name) const
     {
       return operator[] (var_pool.find (name));
diff --git a/build2/target.cxx b/build2/target.cxx
index 9d2f7af..e945d6d 100644
--- a/build2/target.cxx
+++ b/build2/target.cxx
@@ -114,55 +114,26 @@ namespace build2
     return *r;
   }
 
-  lookup<const value> target::
+  lookup target::
   operator[] (const variable& var) const
   {
-    using result = lookup<const value>;
-
     if (auto p = vars.find (var))
-      return result (p, &vars);
+      return lookup (p, &vars);
 
     if (group != nullptr)
     {
       if (auto p = group->vars.find (var))
-        return result (p, &group->vars);
+        return lookup (p, &group->vars);
     }
 
-    // We cannot simply delegate to scope's lookup() since we also need
-    // to check the group.
+    // Delegate to scope's find().
     //
-    for (const scope* s (&base_scope ()); s != nullptr; )
-    {
-      if (!s->target_vars.empty ())
-      {
-        if (auto l = s->target_vars.lookup (type (), name, var))
-          return l;
-
-        if (group != nullptr)
-        {
-          if (auto l = s->target_vars.lookup (group->type (), group->name, var))
-            return l;
-        }
-      }
-
-      if (auto r = s->vars.find (var))
-        return result (r, &s->vars);
-
-      switch (var.visibility)
-      {
-      case variable_visibility::scope:
-        s = nullptr;
-        break;
-      case variable_visibility::project:
-        s = s->root () ? nullptr : s->parent_scope ();
-        break;
-      case variable_visibility::normal:
-        s = s->parent_scope ();
-        break;
-      }
-    }
-
-    return result ();
+    return base_scope ().find (
+      var,
+      &type (),
+      &name,
+      group != nullptr ? &group->type () : nullptr,
+      group != nullptr ? &group->name : nullptr);
   }
 
   value& target::
diff --git a/build2/target.txx b/build2/target.txx
index b7d41f2..fe21016 100644
--- a/build2/target.txx
+++ b/build2/target.txx
@@ -22,7 +22,7 @@ namespace build2
   {
     // Include target type/pattern-specific variables.
     //
-    if (auto l = s.lookup (tk, var))
+    if (auto l = s.find (var, tk))
     {
       // Help the user here and strip leading '.' from the extension.
       //
diff --git a/build2/test/rule.cxx b/build2/test/rule.cxx
index 063d148..2815cf9 100644
--- a/build2/test/rule.cxx
+++ b/build2/test/rule.cxx
@@ -26,7 +26,7 @@ namespace build2
       // this is a test. So take care of that as well.
       //
       bool r (false);
-      lookup<const value> l;
+      lookup l;
 
       // @@ This logic doesn't take into account target type/pattern-
       // specific variables.
@@ -44,7 +44,7 @@ namespace build2
         //
         if (var.name == "test")
         {
-          l = lookup<const value> (val, t);
+          l = lookup (val, t);
           break;
         }
 
@@ -126,6 +126,7 @@ namespace build2
       // First check the target-specific vars since they override any
       // scope ones.
       //
+      //@@ OVR
       auto il (t.vars["test.input"]);
       auto ol (t.vars["test.output"]);
       auto rl (t.vars["test.roundtrip"]);
@@ -162,12 +163,12 @@ namespace build2
         //
         for (scope* s (&bs); s != nullptr; s = s->parent_scope ())
         {
-          ol = s->vars[on];
+          ol = s->vars[on]; //@@ OVR
 
           if (!al) // Not overriden at target level by test.arguments?
           {
-            il = s->vars[in];
-            rl = s->vars[rn];
+            il = s->vars[in]; //@@ OVR
+            rl = s->vars[rn]; //@@ OVR
           }
 
           if (il || ol || rl)
@@ -289,7 +290,7 @@ namespace build2
       string var ("test.");
       var += n;
 
-      auto l (t.vars[var]);
+      auto l (t.vars[var]); //@@ OVR
 
       if (!l)
       {
diff --git a/build2/utility b/build2/utility
index e906b55..e433b5a 100644
--- a/build2/utility
+++ b/build2/utility
@@ -149,13 +149,13 @@ namespace build2
   // As above but from the strings value directly.
   //
   class value;
-  template <typename> struct lookup;
+  struct lookup;
 
   void
-  append_options (cstrings&, const lookup<const value>&);
+  append_options (cstrings&, const lookup&);
 
   void
-  hash_options (sha256&, const lookup<const value>&);
+  hash_options (sha256&, const lookup&);
 
   void
   append_options (cstrings&, const strings&);
@@ -171,7 +171,7 @@ namespace build2
   find_option (const char* option, T&, const char* variable);
 
   bool
-  find_option (const char* option, const lookup<const value>&);
+  find_option (const char* option, const lookup&);
 
   // Parse version string in the X.Y.Z[-{a|b}N] to a version integer in the
   // AABBCCDD form, where:
diff --git a/build2/utility.cxx b/build2/utility.cxx
index 71aed79..8b772e0 100644
--- a/build2/utility.cxx
+++ b/build2/utility.cxx
@@ -144,14 +144,14 @@ namespace build2
   const dir_path empty_dir_path;
 
   void
-  append_options (cstrings& args, const lookup<const value>& l)
+  append_options (cstrings& args, const lookup& l)
   {
     if (l)
       append_options (args, cast<strings> (l));
   }
 
   void
-  hash_options (sha256& csum, const lookup<const value>& l)
+  hash_options (sha256& csum, const lookup& l)
   {
     if (l)
       hash_options (csum, cast<strings> (l));
@@ -177,7 +177,7 @@ namespace build2
   }
 
   bool
-  find_option (const char* option, const lookup<const value>& l)
+  find_option (const char* option, const lookup& l)
   {
     if (l)
     {
diff --git a/build2/variable b/build2/variable
index 7b137aa..15f9c47 100644
--- a/build2/variable
+++ b/build2/variable
@@ -21,7 +21,7 @@ namespace build2
 {
   class value;
   struct variable;
-  template <typename> struct lookup;
+  struct lookup;
 
   struct value_type
   {
@@ -77,6 +77,8 @@ namespace build2
   //
   // The two variables are considered the same if they have the same name.
   //
+  // @@ Document override semantics.
+  //
   struct variable
   {
     string name;
@@ -197,18 +199,14 @@ namespace build2
   template <typename T> T& cast (value&);
   template <typename T> T&& cast (value&&);
   template <typename T> const T& cast (const value&);
-
-  template <typename T> T& cast (const lookup<value>&);
-  template <typename T> const T& cast (const lookup<const value>&);
+  template <typename T> const T& cast (const lookup&);
 
   // As above but returns NULL if the value is NULL (or not defined, in
   // case of lookup).
   //
   template <typename T> T* cast_null (value&);
   template <typename T> const T* cast_null (const value&);
-
-  template <typename T> T* cast_null (const lookup<value>&);
-  template <typename T> const T* cast_null (const lookup<const value>&);
+  template <typename T> const T* cast_null (const lookup&);
 
   // Assign value type to the value. Variable is normally only used for
   // diagnostics.
@@ -230,10 +228,11 @@ namespace build2
   //
   struct variable_map;
 
-  template <typename V>
   struct lookup
   {
-    V* value;
+    using value_type = build2::value;
+
+    const value_type* value;
     const variable_map* vars;
 
     bool
@@ -243,22 +242,23 @@ namespace build2
     //
     explicit operator bool () const {return defined () && !value->null ();}
 
-    V& operator*  () const {return *value;}
-    V* operator-> () const {return value;}
+    const value_type& operator*  () const {return *value;}
+    const value_type* operator-> () const {return value;}
 
     // Return true if this value belongs to the specified scope or target.
-    // Note that it can also be a target type/pattern-specific value.
+    // Note that it can also be a target type/pattern-specific value (in
+    // which case it won't belong to either).
     //
     template <typename T>
     bool
     belongs (const T& x) const {return vars == &x.vars;}
 
     lookup (): value (nullptr), vars (nullptr) {}
-    lookup (V* v, const variable_map* vs)
+    lookup (const value_type* v, const variable_map* vs)
         : value (v), vars (v != nullptr ? vs : nullptr) {}
 
     template <typename T>
-    lookup (V& v, const T& x): lookup (&v, &x.vars) {}
+    lookup (const value_type& v, const T& x): lookup (&v, &x.vars) {}
   };
 
   // Representation types.
@@ -590,32 +590,21 @@ namespace build2
 
     using const_iterator = iterator_adapter<map_type::const_iterator>;
 
-    lookup<const value>
+    // Lookup. Note that variable overrides will not be applied, even if
+    // set in this map.
+    //
+    lookup
     operator[] (const variable& var) const
     {
-      return lookup<const value> (find (var), this);
+      return lookup (find (var), this);
     }
 
-    lookup<const value>
+    lookup
     operator[] (const string& name) const
     {
       return operator[] (var_pool.find (name));
     }
 
-    // Non-const lookup. Only exposed on the map directly.
-    //
-    lookup<value>
-    operator[] (const variable& var)
-    {
-      return lookup<value> (find (var), this);
-    }
-
-    lookup<value>
-    operator[] (const string& name)
-    {
-      return operator[] (var_pool.find (name));
-    }
-
     const value*
     find (const variable&) const;
 
@@ -675,12 +664,13 @@ namespace build2
   //    consider its lifetime.
   //
   using variable_pattern_map = std::map<string, variable_map>;
+  using variable_type_map_base = std::map<reference_wrapper<const target_type>,
+                                          variable_pattern_map>;
 
-  struct variable_type_map: std::map<reference_wrapper<const target_type>,
-                                     variable_pattern_map>
+  struct variable_type_map: variable_type_map_base
   {
-    build2::lookup<const value>
-    lookup (const target_type&, const string& name, const variable&) const;
+    lookup
+    find (const target_type&, const string& tname, const variable&) const;
   };
 }
 
diff --git a/build2/variable.cxx b/build2/variable.cxx
index 4597e9e..0870000 100644
--- a/build2/variable.cxx
+++ b/build2/variable.cxx
@@ -640,18 +640,16 @@ namespace build2
 
   // variable_type_map
   //
-  lookup<const value> variable_type_map::
-  lookup (const target_type& type,
-          const string& name,
-          const variable& var) const
+  lookup variable_type_map::
+  find (const target_type& type,
+        const string& name,
+        const variable& var) const
   {
-    using result = build2::lookup<const value>;
-
     // Search across target type hierarchy.
     //
     for (auto tt (&type); tt != nullptr; tt = tt->base)
     {
-      auto i (find (*tt));
+      auto i (variable_type_map_base::find (*tt));
 
       if (i == end ())
         continue;
@@ -698,11 +696,11 @@ namespace build2
           //@@ TODO: should we detect ambiguity? 'foo-*' '*-foo' and
           //   'foo-foo'? Right now the last defined will be used.
           //
-          return result (v, &j->second);
+          return lookup (v, &j->second);
         }
       }
     }
 
-    return result ();
+    return lookup ();
   }
 }
diff --git a/build2/variable.ixx b/build2/variable.ixx
index a4d7dc3..ee0f647 100644
--- a/build2/variable.ixx
+++ b/build2/variable.ixx
@@ -104,15 +104,8 @@ namespace build2
   }
 
   template <typename T>
-  inline T&
-  cast (const lookup<value>& l)
-  {
-    return cast<T> (*l);
-  }
-
-  template <typename T>
   inline const T&
-  cast (const lookup<const value>& l)
+  cast (const lookup& l)
   {
     return cast<T> (*l);
   }
@@ -132,15 +125,8 @@ namespace build2
   }
 
   template <typename T>
-  inline T*
-  cast_null (const lookup<value>& l)
-  {
-    return l ? &cast<T> (*l) : nullptr;
-  }
-
-  template <typename T>
   inline const T*
-  cast_null (const lookup<const value>& l)
+  cast_null (const lookup& l)
   {
     return l ? &cast<T> (*l) : nullptr;
   }