Add config.bin.target var/hint, use to decide libso{} install mode

Normally the user doesn't need to specify config.bin.target explicitly
since the cxx module will hint it. We now also have the whole set of
target's components:

bin.target.{cpu,vendor,system,version,class}

14 files changed, 352 insertions, 175 deletions

diff --git a/build2/b.cxx b/build2/b.cxx
index e7c8721..713b227 100644
--- a/build2/b.cxx
+++ b/build2/b.cxx
@@ -792,7 +792,7 @@ main (int argc, char* argv[])
           bool first (true);
           for (const variable_override& o: var_ovs)
           {
-            auto p (rs.vars.assign (o.ovr));
+            auto p (rs.vars.insert (o.ovr));
 
             if (!p.second)
             {
diff --git a/build2/bin/module.cxx b/build2/bin/module.cxx
index 70a2b98..5e668df 100644
--- a/build2/bin/module.cxx
+++ b/build2/bin/module.cxx
@@ -4,6 +4,8 @@
 
 #include <build2/bin/module>
 
+#include <butl/triplet>
+
 #include <build2/scope>
 #include <build2/variable>
 #include <build2/diagnostics>
@@ -16,6 +18,7 @@
 #include <build2/bin/target>
 
 using namespace std;
+using namespace butl;
 
 namespace build2
 {
@@ -33,7 +36,7 @@ namespace build2
     bool
     init (scope& r,
           scope& b,
-          const location&,
+          const location& loc,
           unique_ptr<module_base>&,
           bool first,
           bool,
@@ -42,8 +45,6 @@ namespace build2
       tracer trace ("bin::init");
       l5 ([&]{trace << "for " << b.out_path ();});
 
-      assert (config_hints.empty ()); // We don't known any hints.
-
       // Enter module variables.
       //
       if (first)
@@ -52,6 +53,8 @@ namespace build2
 
         // Note: some overridable, some not.
         //
+        v.insert<string>    ("config.bin.target",    true);
+
         v.insert<path>      ("config.bin.ar",        true);
         v.insert<path>      ("config.bin.ranlib",    true);
 
@@ -70,45 +73,6 @@ namespace build2
         v.insert<string>    ("bin.libprefix",        true);
       }
 
-      // Register target types.
-      //
-      {
-        auto& t (b.target_types);
-
-        t.insert<obja>  ();
-        t.insert<objso> ();
-        t.insert<obj>   ();
-        t.insert<exe>   ();
-        t.insert<liba>  ();
-        t.insert<libso> ();
-        t.insert<lib>   ();
-      }
-
-      // Register rules.
-      //
-      {
-        auto& r (b.rules);
-
-        r.insert<obj> (perform_update_id, "bin.obj", obj_);
-        r.insert<obj> (perform_clean_id, "bin.obj", obj_);
-
-        r.insert<lib> (perform_update_id, "bin.lib", lib_);
-        r.insert<lib> (perform_clean_id, "bin.lib", lib_);
-
-        // Configure member.
-        //
-        r.insert<lib> (configure_update_id, "bin.lib", lib_);
-
-        //@@ Should we check if the install module was loaded
-        //   (by checking if install operation is registered
-        //   for this project)? If we do that, then install
-        //   will have to be loaded before bin. Perhaps we
-        //   should enforce loading of all operation-defining
-        //   modules before all others?
-        //
-        r.insert<lib> (perform_install_id, "bin.lib", lib_);
-      }
-
       // Configure.
       //
       using config::required;
@@ -163,15 +127,99 @@ namespace build2
       b.assign ("bin.rpath") += cast_null<dir_paths> (
         config::optional (r, "config.bin.rpath"));
 
-      // config.bin.ar
-      // config.bin.ranlib
-      //
-      // For config.bin.ar we default to 'ar' while ranlib should be explicitly
-      // specified by the user in order for us to use it (most targets support
-      // the -s option to ar).
-      //
       if (first)
       {
+        // config.bin.target
+        //
+        {
+          const variable& cbt (var_pool.find ("config.bin.target"));
+
+          // We first see if the value was specified via the configuration
+          // mechanism.
+          //
+          auto p (config::required (r, cbt));
+          const value* v (p.first);
+
+          // Then see if there is a config hint (e.g., from the C++ module).
+          //
+          bool hint (false);
+          if (v == nullptr)
+          {
+            if (auto l = config_hints[cbt])
+            {
+              v = l.value;
+              hint = true;
+            }
+          }
+
+          if (v == nullptr)
+            fail (loc) << "unable to determine binutils target" <<
+              info << "consider specifying it with config.bin.target" <<
+              info << "or first load a module that can provide it as a hint, "
+                       << "such as c or cxx";
+
+          // Split/canonicalize the target.
+          //
+          string s (cast<string> (*v));
+
+          // Did the user ask us to use config.sub? If this is a hinted value,
+          // then we assume it has already been passed through config.sub.
+          //
+          if (!hint && ops.config_sub_specified ())
+          {
+            s = run<string> (ops.config_sub (),
+                             s.c_str (),
+                             [] (string& l) {return move (l);});
+            l5 ([&]{trace << "config.sub target: '" << s << "'";});
+          }
+
+          try
+          {
+            string canon;
+            triplet t (s, canon);
+
+            l5 ([&]{trace << "canonical target: '" << canon << "'; "
+                          << "class: " << t.class_;});
+
+            assert (!hint || s == canon);
+
+            // Enter as bin.target.{cpu,vendor,system,version,class}.
+            //
+            r.assign<string> ("bin.target") = move (canon);
+            r.assign<string> ("bin.target.cpu") = move (t.cpu);
+            r.assign<string> ("bin.target.vendor") = move (t.vendor);
+            r.assign<string> ("bin.target.system") = move (t.system);
+            r.assign<string> ("bin.target.version") = move (t.version);
+            r.assign<string> ("bin.target.class") = move (t.class_);
+          }
+          catch (const invalid_argument& e)
+          {
+            // This is where we suggest that the user specifies --config-sub
+            // to help us out.
+            //
+            fail << "unable to parse binutils target '" << s << "': "
+                 << e.what () <<
+              info << "consider using the --config-sub option";
+          }
+
+          // If this is a new value (e.g., we are configuring), then print the
+          // report at verbosity level 2 and up (-v). Note that a hinted value
+          // will automatically only be printed at level 3 and up.
+          //
+          if (verb >= (p.second ? 2 : 3))
+          {
+            text << "bin\n"
+                 << "  target     " << cast<string> (r["bin.target"]);
+          }
+        }
+
+        // config.bin.ar
+        // config.bin.ranlib
+        //
+        // For config.bin.ar we default to 'ar' while ranlib should be
+        // explicitly specified by the user in order for us to use it (most
+        // targets support the -s option to ar).
+        //
         auto p (config::required (r, "config.bin.ar", path ("ar")));
         auto& v (config::optional (r, "config.bin.ranlib"));
 
@@ -187,16 +235,18 @@ namespace build2
         {
           //@@ Print project out root or name? See cxx.
 
-          text << ar << ":\n"
-               << "  id         " << bi.ar_id << "\n"
-               << "  signature  " << bi.ar_signature << "\n"
+          text << "bin.ar\n"
+               << "  exe        " << ar << '\n'
+               << "  id         " << bi.ar_id << '\n'
+               << "  signature  " << bi.ar_signature << '\n'
                << "  checksum   " << bi.ar_checksum;
 
           if (!ranlib.empty ())
           {
-            text << ranlib << ":\n"
-                 << "  id         " << bi.ranlib_id << "\n"
-                 << "  signature  " << bi.ranlib_signature << "\n"
+            text << "bin.ranlib\n"
+                 << "  exe        " << ranlib << '\n'
+                 << "  id         " << bi.ranlib_id << '\n'
+                 << "  signature  " << bi.ranlib_signature << '\n'
                  << "  checksum   " << bi.ranlib_checksum;
           }
         }
@@ -214,6 +264,49 @@ namespace build2
         }
       }
 
+      // Cache some config values we will be needing below.
+      //
+      const string& tclass (cast<string> (r["bin.target.class"]));
+
+      // Register target types.
+      //
+      {
+        auto& t (b.target_types);
+
+        t.insert<obja>  ();
+        t.insert<objso> ();
+        t.insert<obj>   ();
+        t.insert<exe>   ();
+        t.insert<liba>  ();
+        t.insert<libso> ();
+        t.insert<lib>   ();
+      }
+
+      // Register rules.
+      //
+      {
+        auto& r (b.rules);
+
+        r.insert<obj> (perform_update_id, "bin.obj", obj_);
+        r.insert<obj> (perform_clean_id, "bin.obj", obj_);
+
+        r.insert<lib> (perform_update_id, "bin.lib", lib_);
+        r.insert<lib> (perform_clean_id, "bin.lib", lib_);
+
+        // Configure member.
+        //
+        r.insert<lib> (configure_update_id, "bin.lib", lib_);
+
+        //@@ Should we check if the install module was loaded
+        //   (by checking if install operation is registered
+        //   for this project)? If we do that, then install
+        //   will have to be loaded before bin. Perhaps we
+        //   should enforce loading of all operation-defining
+        //   modules before all others?
+        //
+        r.insert<lib> (perform_install_id, "bin.lib", lib_);
+      }
+
       // Configure "installability" of our target types.
       //
       using namespace install;
@@ -236,11 +329,13 @@ namespace build2
       //
       // libso{foo}: install.mode=755
       //
-      // Everyone is happy then?
+      // Everyone is happy then? Not Windows users. When targeting Windows
+      // libso{} is an import library and shouldn't be exec.
       //
       install_path<libso> (b, dir_path ("lib")); // Install into install.lib.
 
-      //@@ For Windows, libso{} is an import library and shouldn't be exec.
+      if (tclass == "windows")
+        install_mode<libso> (b, "644");
 
       install_path<liba> (b, dir_path ("lib"));  // Install into install.lib.
       install_mode<liba> (b, "644");
diff --git a/build2/config/utility b/build2/config/utility
index beab758..c47cecf 100644
--- a/build2/config/utility
+++ b/build2/config/utility
@@ -55,6 +55,21 @@ namespace build2
       return required (root, name, string (default_value), override);
     }
 
+    // As above, but leave the unspecified value as undefined (and return
+    // NULL pointer) rather than setting it to the default value.
+    //
+    // This can be useful when we don't have a default value but may figure
+    // out some fallback. See config.bin.target for an example.
+    //
+    pair<const value*, bool>
+    required (scope& root, const variable&);
+
+    inline pair<const value*, bool>
+    required (scope& root, const string& name)
+    {
+      return required (root, var_pool.find (name));
+    }
+
     // Set, if necessary, an optional config.* variable. In particular,
     // an unspecified variable is set to NULL which is used to distinguish
     // between the "configured as unspecified" and "not yet configured"
diff --git a/build2/config/utility.cxx b/build2/config/utility.cxx
index 1dbf3d3..768a70d 100644
--- a/build2/config/utility.cxx
+++ b/build2/config/utility.cxx
@@ -15,28 +15,63 @@ namespace build2
   namespace config
   {
     void
-    save_variable (scope& root, const variable& var, uint64_t flags)
+    save_variable (scope& r, const variable& var, uint64_t flags)
     {
       if (current_mif->id == configure_id)
       {
         // The project might not be using the config module. But then how
         // could we be configuring it? Good question.
         //
-        if (module* mod = root.modules.lookup<module> (module::name))
+        if (module* mod = r.modules.lookup<module> (module::name))
           mod->vars.emplace (var, flags);
       }
     }
 
+    pair<const value*, bool>
+    required (scope& r, const variable& var)
+    {
+      // This is a stripped-down version of the other required() twisted
+      // implementation.
+
+      pair<lookup, size_t> org (r.find_original (var));
+
+      bool n (false); // New flag.
+      lookup l (org.first);
+
+      // Treat an inherited value that was set to default as new.
+      //
+      if (l.defined () && l->extra)
+        n = true;
+
+      if (var.override != nullptr)
+      {
+        pair<lookup, size_t> ovr (r.find_override (var, move (org)));
+
+        if (l != ovr.first) // Overriden?
+        {
+          // Override is always treated as new.
+          //
+          n = true;
+          l = move (ovr.first);
+        }
+      }
+
+      if (l.defined () && current_mif->id == configure_id)
+          save_variable (r, var);
+
+      return pair<const value*, bool> (l.value, n);
+    }
+
     const value&
-    optional (scope& root, const variable& var)
+    optional (scope& r, const variable& var)
     {
       if (current_mif->id == configure_id)
-        save_variable (root, var);
+        save_variable (r, var);
 
-      auto l (root[var]);
+      auto l (r[var]);
       return l.defined ()
         ? *l
-        : root.assign (var); // NULL.
+        : r.assign (var); // NULL.
     }
 
     bool
diff --git a/build2/config/utility.txx b/build2/config/utility.txx
index 30ed02f..5ebd261 100644
--- a/build2/config/utility.txx
+++ b/build2/config/utility.txx
@@ -13,6 +13,9 @@ namespace build2
     pair<reference_wrapper<const value>, bool>
     required (scope& root, const variable& var, const T& def_val, bool def_ovr)
     {
+      // Note: see also the other required() version if changing anything
+      // here.
+
       if (current_mif->id == configure_id)
         save_variable (root, var);
 
diff --git a/build2/context.cxx b/build2/context.cxx
index 3e9fe4a..59eb912 100644
--- a/build2/context.cxx
+++ b/build2/context.cxx
@@ -43,6 +43,9 @@ namespace build2
   {
     tracer trace ("reset");
 
+    // @@ Need to unload modules when we dynamically load them.
+    //
+
     l6 ([&]{trace << "resetting build state";});
 
     variable_overrides vos;
@@ -183,7 +186,7 @@ namespace build2
       if (c == '!' || !dir.empty ())
       {
         scope& s (c == '!' ? gs : scopes.insert (dir, false)->second);
-        auto p (s.vars.assign (*o));
+        auto p (s.vars.insert (*o));
 
         if (!p.second)
         {
diff --git a/build2/cxx/module.cxx b/build2/cxx/module.cxx
index 71304c8..aa60be9 100644
--- a/build2/cxx/module.cxx
+++ b/build2/cxx/module.cxx
@@ -41,22 +41,8 @@ namespace build2
       tracer trace ("cxx::init");
       l5 ([&]{trace << "for " << b.out_path ();});
 
-      assert (config_hints.empty ()); // We don't known any hints.
-
-      // Initialize the bin module. Only do this if it hasn't already been
-      // loaded so that we don't overwrite user's bin.* settings.
-      //
-      if (!cast_false<bool> (b["bin.loaded"]))
-        load_module ("bin", r, b, loc);
-
       // Enter module variables.
       //
-      // @@ Probably should only be done on load; make sure reset() unloads
-      //    modules.
-      //
-      // @@ Should probably cache the variable pointers so we don't have
-      //    to keep looking them up.
-      //
       if (first)
       {
         auto& v (var_pool);
@@ -82,64 +68,11 @@ namespace build2
         v.insert<string> ("cxx.std",              true);
       }
 
-      // Register target types.
-      //
-      {
-        auto& t (b.target_types);
-
-        t.insert<h> ();
-        t.insert<c> ();
-
-        t.insert<cxx> ();
-        t.insert<hxx> ();
-        t.insert<ixx> ();
-        t.insert<txx> ();
-      }
-
-      // Register rules.
-      //
-      {
-        using namespace bin;
-
-        auto& r (b.rules);
-
-        r.insert<obja> (perform_update_id, "cxx.compile", compile::instance);
-
-        r.insert<obja> (perform_update_id, "cxx.compile", compile::instance);
-        r.insert<obja> (perform_clean_id, "cxx.compile", compile::instance);
-
-        r.insert<objso> (perform_update_id, "cxx.compile", compile::instance);
-        r.insert<objso> (perform_clean_id, "cxx.compile", compile::instance);
-
-        r.insert<exe> (perform_update_id, "cxx.link", link::instance);
-        r.insert<exe> (perform_clean_id, "cxx.link", link::instance);
-
-        r.insert<liba> (perform_update_id, "cxx.link", link::instance);
-        r.insert<liba> (perform_clean_id, "cxx.link", link::instance);
-
-        r.insert<libso> (perform_update_id, "cxx.link", link::instance);
-        r.insert<libso> (perform_clean_id, "cxx.link", link::instance);
-
-        // Register for configure so that we detect unresolved imports
-        // during configuration rather that later, e.g., during update.
-        //
-        r.insert<obja> (configure_update_id, "cxx.compile", compile::instance);
-        r.insert<objso> (configure_update_id, "cxx.compile", compile::instance);
-
-        r.insert<exe> (configure_update_id, "cxx.link", link::instance);
-        r.insert<liba> (configure_update_id, "cxx.link", link::instance);
-        r.insert<libso> (configure_update_id, "cxx.link", link::instance);
-
-        //@@ Should we check if install module was loaded (see bin)?
-        //
-        r.insert<exe> (perform_install_id, "cxx.install", install::instance);
-        r.insert<liba> (perform_install_id, "cxx.install", install::instance);
-        r.insert<libso> (perform_install_id, "cxx.install", install::instance);
-      }
-
       // Configure.
       //
 
+      assert (config_hints.empty ()); // We don't known any hints.
+
       // config.cxx.{p,c,l}options
       // config.cxx.libs
       //
@@ -186,15 +119,16 @@ namespace build2
         {
           //@@ Print project out root or name? Don't print if unnamed?
 
-          text << cxx << ":\n"
-               << "  id         " << ci.id << "\n"
-               << "  version    " << ci.version.string << "\n"
-               << "  major      " << ci.version.major << "\n"
-               << "  minor      " << ci.version.minor << "\n"
-               << "  patch      " << ci.version.patch << "\n"
-               << "  build      " << ci.version.build << "\n"
-               << "  signature  " << ci.signature << "\n"
-               << "  checksum   " << ci.checksum << "\n"
+          text << "cxx\n"
+               << "  exe        " << cxx << '\n'
+               << "  id         " << ci.id << '\n'
+               << "  version    " << ci.version.string << '\n'
+               << "  major      " << ci.version.major << '\n'
+               << "  minor      " << ci.version.minor << '\n'
+               << "  patch      " << ci.version.patch << '\n'
+               << "  build      " << ci.version.build << '\n'
+               << "  signature  " << ci.signature << '\n'
+               << "  checksum   " << ci.checksum << '\n'
                << "  target     " << ci.target;
         }
 
@@ -243,8 +177,8 @@ namespace build2
         }
         catch (const invalid_argument& e)
         {
-          // This is where we could suggest that the user specifies
-          // --config-sub to help us out.
+          // This is where we suggest that the user specifies --config-sub to
+          // help us out.
           //
           fail << "unable to parse compiler target '" << ci.target << "': "
                << e.what () <<
@@ -252,6 +186,90 @@ namespace build2
         }
       }
 
+      // Initialize the bin module. Only do this if it hasn't already been
+      // loaded so that we don't overwrite user's bin.* settings.
+      //
+      const string& target (cast<string> (r["cxx.target"]));
+
+      if (!cast_false<bool> (b["bin.loaded"]))
+      {
+        // Pass the target we extracted from the C++ compiler as a config hint
+        // to the bin module.
+        //
+        variable_map hints;
+        hints.assign ("config.bin.target") = target;
+
+        load_module ("bin", r, b, loc, false, hints);
+      }
+
+      // Verify bin's target matches ours.
+      //
+      {
+        const string& bt (cast<string> (r["bin.target"]));
+
+        if (bt != target)
+          fail (loc) << "bin and cxx module target platform mismatch" <<
+            info << "bin.target is " << bt <<
+            info << "cxx.target is " << target;
+      }
+
+      // Register target types.
+      //
+      {
+        auto& t (b.target_types);
+
+        t.insert<h> ();
+        t.insert<c> ();
+
+        t.insert<cxx> ();
+        t.insert<hxx> ();
+        t.insert<ixx> ();
+        t.insert<txx> ();
+      }
+
+      // Register rules.
+      //
+      {
+        using namespace bin;
+
+        auto& r (b.rules);
+
+        r.insert<obja> (perform_update_id, "cxx.compile", compile::instance);
+
+        r.insert<obja> (perform_update_id, "cxx.compile", compile::instance);
+        r.insert<obja> (perform_clean_id, "cxx.compile", compile::instance);
+
+        r.insert<objso> (perform_update_id, "cxx.compile", compile::instance);
+        r.insert<objso> (perform_clean_id, "cxx.compile", compile::instance);
+
+        r.insert<exe> (perform_update_id, "cxx.link", link::instance);
+        r.insert<exe> (perform_clean_id, "cxx.link", link::instance);
+
+        r.insert<liba> (perform_update_id, "cxx.link", link::instance);
+        r.insert<liba> (perform_clean_id, "cxx.link", link::instance);
+
+        r.insert<libso> (perform_update_id, "cxx.link", link::instance);
+        r.insert<libso> (perform_clean_id, "cxx.link", link::instance);
+
+        // Register for configure so that we detect unresolved imports
+        // during configuration rather that later, e.g., during update.
+        //
+        r.insert<obja> (configure_update_id, "cxx.compile", compile::instance);
+        r.insert<objso> (configure_update_id, "cxx.compile", compile::instance);
+
+        r.insert<exe> (configure_update_id, "cxx.link", link::instance);
+        r.insert<liba> (configure_update_id, "cxx.link", link::instance);
+        r.insert<libso> (configure_update_id, "cxx.link", link::instance);
+
+        //@@ Should we check if install module was loaded (see bin)?
+        //
+        r.insert<exe> (perform_install_id, "cxx.install", install::instance);
+        r.insert<liba> (perform_install_id, "cxx.install", install::instance);
+        r.insert<libso> (perform_install_id, "cxx.install", install::instance);
+      }
+
+
+
       // Configure "installability" of our target types.
       //
       using namespace install;
diff --git a/build2/file.cxx b/build2/file.cxx
index 8b6291a..3195af7 100644
--- a/build2/file.cxx
+++ b/build2/file.cxx
@@ -471,7 +471,7 @@ namespace build2
     // amalgamated.
     //
     {
-      auto rp (root.vars.assign ("amalgamation")); // Set NULL by default.
+      auto rp (root.vars.insert ("amalgamation")); // Set NULL by default.
       value& v (rp.first);
 
       if (v && v.empty ()) // Convert empty to NULL.
@@ -543,7 +543,7 @@ namespace build2
     //
     {
       const variable& var (var_pool.find ("subprojects"));
-      auto rp (root.vars.assign(var)); // Set NULL by default.
+      auto rp (root.vars.insert (var)); // Set NULL by default.
       value& v (rp.first);
 
       if (rp.second)
diff --git a/build2/install/utility b/build2/install/utility
index 1abed27..a50ab9b 100644
--- a/build2/install/utility
+++ b/build2/install/utility
@@ -19,7 +19,7 @@ namespace build2
     inline void
     install_path (const target_type& tt, scope& s, dir_path d)
     {
-      auto r (s.target_vars[tt]["*"].assign ("install"));
+      auto r (s.target_vars[tt]["*"].insert ("install"));
       if (r.second) // Already set by the user?
         r.first.get () = move (d);
     }
@@ -34,7 +34,7 @@ namespace build2
     inline void
     install_mode (const target_type& tt, scope& s, string m)
     {
-      auto r (s.target_vars[tt]["*"].assign ("install.mode"));
+      auto r (s.target_vars[tt]["*"].insert ("install.mode"));
       if (r.second) // Already set by the user?
         r.first.get () = move (m);
     }
diff --git a/build2/parser.cxx b/build2/parser.cxx
index 1edc7ba..b85349b 100644
--- a/build2/parser.cxx
+++ b/build2/parser.cxx
@@ -494,8 +494,8 @@ namespace build2
                   // Note: expanding the value in the context of the scope.
                   //
                   value rhs (variable_value (t, tt));
-                  value& lhs (scope_->target_vars[*ti][move (n.value)].assign (
-                                var).first);
+                  value& lhs (
+                    scope_->target_vars[*ti][move (n.value)].assign (var));
                   value_attributes (&var, lhs, move (rhs), type::assign);
                 }
               }
diff --git a/build2/scope b/build2/scope
index 3649aa1..b7c9aef 100644
--- a/build2/scope
+++ b/build2/scope
@@ -154,17 +154,17 @@ namespace build2
     // is returned.
     //
     value&
-    assign (const variable& var) {return vars.assign (var).first.get ();}
+    assign (const variable& var) {return vars.assign (var);}
 
     value&
-    assign (const string& name) {return vars.assign (name).first.get ();}
+    assign (const string& name) {return vars.assign (name);}
 
-    // Unlike the two above, assign a non-overridable variable with normal
-    // visibility.
+    // Unlike the two above, assign a typed non-overridable variable with
+    // normal visibility.
     //
     template <typename T>
     value&
-    assign (string name) {return vars.assign<T> (move (name)).first.get ();}
+    assign (string name) {return vars.assign<T> (move (name));}
 
     // Return a value suitable for appending. If the variable does not
     // exist in this scope's map, then outer scopes are searched for
diff --git a/build2/target b/build2/target
index 0205052..3207778 100644
--- a/build2/target
+++ b/build2/target
@@ -383,13 +383,13 @@ namespace build2
     // Return a value suitable for assignment. See scope for details.
     //
     value&
-    assign (const variable& var) {return vars.assign (var).first;}
+    assign (const variable& var) {return vars.assign (var);}
 
     value&
-    assign (const string& name) {return vars.assign (name).first;}
+    assign (const string& name) {return vars.assign (name);}
 
-    // Unlike the two above, assign a non-overridable variable with normal
-    // visibility.
+    // Unlike the two above, assign a typed non-overridable variable with
+    // normal visibility.
     //
     template <typename T>
     value&
diff --git a/build2/variable b/build2/variable
index 5c567da..adc11fa 100644
--- a/build2/variable
+++ b/build2/variable
@@ -280,13 +280,13 @@ namespace build2
   // A variable can be undefined, NULL, or contain a (potentially empty)
   // value.
   //
-  struct variable_map;
+  class variable_map;
 
   struct lookup
   {
     using value_type = build2::value;
 
-    const value_type* value;
+    const value_type* value;  // NULL if undefined.
     const variable_map* vars;
 
     bool
@@ -701,28 +701,36 @@ namespace build2
     value*
     find (const variable&);
 
-    // The second member in the pair indicates whether the new value (which
-    // will be NULL) was assigned.
+    // Return a value suitable for assignment. See scope for details.
     //
-    pair<reference_wrapper<value>, bool>
-    assign (const variable&);
+    value&
+    assign (const variable& var) {return insert (var).first;}
 
-    pair<reference_wrapper<value>, bool>
-    assign (const string& name)
-    {
-      return assign (var_pool.find (name));
-    }
+    value&
+    assign (const string& name) {return insert (name).first;}
 
-    // Unlike the two above, assign a non-overridable variable with normal
-    // visibility.
+    // Unlike the two above, assign a typed, non-overridable variable with
+    // normal visibility.
     //
     template <typename T>
-    pair<reference_wrapper<value>, bool>
+    value&
     assign (string name)
     {
       return assign (var_pool.insert<T> (move (name)));
     }
 
+    // As above but also return an indication of whether the new value (which
+    // will be NULL) was actually inserted.
+    //
+    pair<reference_wrapper<value>, bool>
+    insert (const variable&);
+
+    pair<reference_wrapper<value>, bool>
+    insert (const string& name)
+    {
+      return insert (var_pool.find (name));
+    }
+
     pair<const_iterator, const_iterator>
     find_namespace (const string& ns) const
     {
diff --git a/build2/variable.cxx b/build2/variable.cxx
index fdee0dd..17a9d52 100644
--- a/build2/variable.cxx
+++ b/build2/variable.cxx
@@ -694,7 +694,7 @@ namespace build2
   }
 
   pair<reference_wrapper<value>, bool> variable_map::
-  assign (const variable& var)
+  insert (const variable& var)
   {
     auto r (m_.emplace (var, value (var.type)));
     value& v (r.first->second);