Improve module loading API

16 files changed, 125 insertions, 113 deletions

diff --git a/libbuild2/bash/init.cxx b/libbuild2/bash/init.cxx
index 65d9d0f..dc1f8f6 100644
--- a/libbuild2/bash/init.cxx
+++ b/libbuild2/bash/init.cxx
@@ -37,8 +37,7 @@ namespace build2
 
       // Load in.base (in.* variables, in{} target type).
       //
-      if (!cast_false<bool> (rs["in.base.loaded"]))
-        load_module (rs, rs, "in.base", l);
+      load_module (rs, rs, "in.base", l);
 
       bool install_loaded (cast_false<bool> (rs["install.loaded"]));
 
diff --git a/libbuild2/bin/init.cxx b/libbuild2/bin/init.cxx
index 1e5ccc5..344d54e 100644
--- a/libbuild2/bin/init.cxx
+++ b/libbuild2/bin/init.cxx
@@ -161,8 +161,7 @@ namespace build2
 
       // Load bin.vars.
       //
-      if (!cast_false<bool> (rs["bin.vars.loaded"]))
-        load_module (rs, rs, "bin.vars", loc);
+      load_module (rs, rs, "bin.vars", loc);
 
       // Configure.
       //
@@ -423,8 +422,7 @@ namespace build2
 
       // Load bin.config.
       //
-      if (!cast_false<bool> (rs["bin.config.loaded"]))
-        load_module (rs, rs, "bin.config", loc, false, hints);
+      load_module (rs, rs, "bin.config", loc, hints);
 
       // Cache some config values we will be needing below.
       //
@@ -551,7 +549,7 @@ namespace build2
           r.insert<lib> (perform_uninstall_id, "bin.lib", gr);
         }
 
-        if (const test::module* m = rs.lookup_module<test::module> ("test"))
+        if (const test::module* m = rs.find_module<test::module> ("test"))
         {
           r.insert<lib> (perform_test_id, "bin.lib", m->group_rule ());
         }
@@ -574,8 +572,7 @@ namespace build2
 
       // Make sure bin.config is loaded.
       //
-      if (!cast_false<bool> (rs["bin.config.loaded"]))
-        load_module (rs, bs, "bin.config", loc, false, hints);
+      load_module (rs, bs, "bin.config", loc, hints);
 
       // Enter configuration variables.
       //
@@ -723,11 +720,8 @@ namespace build2
 
       // Make sure the bin core and ar.config are loaded.
       //
-      if (!cast_false<bool> (bs["bin.loaded"]))
-        load_module (rs, bs, "bin", loc, false, hints);
-
-      if (!cast_false<bool> (bs["bin.ar.config.loaded"]))
-        load_module (rs, bs, "bin.ar.config", loc, false, hints);
+      load_module (rs, bs, "bin",           loc, hints);
+      load_module (rs, bs, "bin.ar.config", loc, hints);
 
       return true;
     }
@@ -746,8 +740,7 @@ namespace build2
 
       // Make sure bin.config is loaded.
       //
-      if (!cast_false<bool> (rs["bin.config.loaded"]))
-        load_module (rs, rs, "bin.config", loc, false, hints);
+      load_module (rs, rs, "bin.config", loc, hints);
 
       // Enter configuration variables.
       //
@@ -820,11 +813,8 @@ namespace build2
 
       // Make sure the bin core and ld.config are loaded.
       //
-      if (!cast_false<bool> (bs["bin.loaded"]))
-        load_module (rs, bs, "bin", loc, false, hints);
-
-      if (!cast_false<bool> (bs["bin.ld.config.loaded"]))
-        load_module (rs, bs, "bin.ld.config", loc, false, hints);
+      load_module (rs, bs, "bin",           loc, hints);
+      load_module (rs, bs, "bin.ld.config", loc, hints);
 
       const string& lid (cast<string> (rs["bin.ld.id"]));
 
@@ -856,8 +846,7 @@ namespace build2
 
       // Make sure bin.config is loaded.
       //
-      if (!cast_false<bool> (bs["bin.config.loaded"]))
-        load_module (rs, bs, "bin.config", loc, false, hints);
+      load_module (rs, bs, "bin.config", loc, hints);
 
       // Enter configuration variables.
       //
@@ -930,11 +919,8 @@ namespace build2
 
       // Make sure the bin core and rc.config are loaded.
       //
-      if (!cast_false<bool> (bs["bin.loaded"]))
-        load_module (rs, bs, "bin", loc, false, hints);
-
-      if (!cast_false<bool> (bs["bin.rc.config.loaded"]))
-        load_module (rs, bs, "bin.rc.config", loc, false, hints);
+      load_module (rs, bs, "bin",           loc, hints);
+      load_module (rs, bs, "bin.rc.config", loc, hints);
 
       return true;
     }
diff --git a/libbuild2/c/init.cxx b/libbuild2/c/init.cxx
index 423e6af..4efba69 100644
--- a/libbuild2/c/init.cxx
+++ b/libbuild2/c/init.cxx
@@ -146,8 +146,7 @@ namespace build2
 
       // Load cc.core.vars so that we can cache all the cc.* variables.
       //
-      if (!cast_false<bool> (rs["cc.core.vars.loaded"]))
-        load_module (rs, rs, "cc.core.vars", loc);
+      load_module (rs, rs, "cc.core.vars", loc);
 
       // Enter all the variables and initialize the module data.
       //
@@ -285,10 +284,8 @@ namespace build2
 
       // Load c.guess.
       //
-      if (!cast_false<bool> (rs["c.guess.loaded"]))
-        load_module (rs, rs, "c.guess", loc, false, hints);
+      auto& cm (load_module<config_module> (rs, rs, "c.guess", loc, hints));
 
-      config_module& cm (*rs.lookup_module<config_module> ("c.guess"));
       cm.init (rs, loc, hints);
       return true;
     }
@@ -325,10 +322,11 @@ namespace build2
 
       // Load c.config.
       //
-      if (!cast_false<bool> (rs["c.config.loaded"]))
-        load_module (rs, rs, "c.config", loc, false, hints);
+      // @@ TODO: move guess to config and use return value?
+      //
+      load_module (rs, rs, "c.config", loc, hints);
 
-      config_module& cm (*rs.lookup_module<config_module> ("c.guess"));
+      config_module& cm (*rs.find_module<config_module> ("c.guess"));
 
       cc::data d {
         cm,
diff --git a/libbuild2/cc/compile-rule.cxx b/libbuild2/cc/compile-rule.cxx
index 328b65e..de35ad5 100644
--- a/libbuild2/cc/compile-rule.cxx
+++ b/libbuild2/cc/compile-rule.cxx
@@ -5201,7 +5201,7 @@ namespace build2
       //
 #ifndef NDEBUG
       assert (ps->root ());
-      const module* m (ps->lookup_module<module> (x));
+      const module* m (ps->find_module<module> (x));
       assert (m != nullptr && m->modules);
 #endif
 
diff --git a/libbuild2/cc/init.cxx b/libbuild2/cc/init.cxx
index d310c6b..a495519 100644
--- a/libbuild2/cc/init.cxx
+++ b/libbuild2/cc/init.cxx
@@ -74,8 +74,7 @@ namespace build2
 
       // Load bin.vars (we need its config.bin.target/pattern for hints).
       //
-      if (!cast_false<bool> (rs["bin.vars.loaded"]))
-        load_module (rs, rs, "bin.vars", loc);
+      load_module (rs, rs, "bin.vars", loc);
 
       // Enter variables. Note: some overridable, some not.
       //
@@ -172,8 +171,7 @@ namespace build2
 
       // Load cc.core.vars.
       //
-      if (!cast_false<bool> (rs["cc.core.vars.loaded"]))
-        load_module (rs, rs, "cc.core.vars", loc);
+      load_module (rs, rs, "cc.core.vars", loc);
 
       // config.cc.{id,hinter}
       //
@@ -246,8 +244,7 @@ namespace build2
 
       // Load cc.core.guess.
       //
-      if (!cast_false<bool> (rs["cc.core.guess.loaded"]))
-        load_module (rs, rs, "cc.core.guess", loc);
+      load_module (rs, rs, "cc.core.guess", loc);
 
       // Configure.
       //
@@ -303,7 +300,7 @@ namespace build2
             h.assign ("config.bin.pattern") = cast<string> (l);
         }
 
-        load_module (rs, rs, "bin.config", loc, false, h);
+        init_module (rs, rs, "bin.config", loc, false /* optional */, h);
       }
 
       // Verify bin's target matches ours (we do it even if we loaded it
@@ -330,20 +327,13 @@ namespace build2
       //
       const string& tsys (cast<string> (rs["cc.target.system"]));
 
-      if (!cast_false<bool> (rs["bin.ar.config.loaded"]))
-        load_module (rs, rs, "bin.ar.config", loc);
+      load_module (rs, rs, "bin.ar.config", loc);
 
       if (tsys == "win32-msvc")
-      {
-        if (!cast_false<bool> (rs["bin.ld.config.loaded"]))
-          load_module (rs, rs, "bin.ld.config", loc);
-      }
+        load_module (rs, rs, "bin.ld.config", loc);
 
       if (tsys == "mingw32")
-      {
-        if (!cast_false<bool> (rs["bin.rc.config.loaded"]))
-          load_module (rs, rs, "bin.rc.config", loc);
-      }
+        load_module (rs, rs, "bin.rc.config", loc);
 
       return true;
     }
@@ -366,36 +356,27 @@ namespace build2
 
       // Load cc.core.config.
       //
-      if (!cast_false<bool> (rs["cc.core.config.loaded"]))
-        load_module (rs, rs, "cc.core.config", loc, false, hints);
+      load_module (rs, rs, "cc.core.config", loc, hints);
 
       // Load the bin module.
       //
-      if (!cast_false<bool> (rs["bin.loaded"]))
-        load_module (rs, rs, "bin", loc);
+      load_module (rs, rs, "bin", loc);
 
       // Load the bin.ar module.
       //
-      if (!cast_false<bool> (rs["bin.ar.loaded"]))
-        load_module (rs, rs, "bin.ar", loc);
+      load_module (rs, rs, "bin.ar", loc);
 
       // For this target we link things directly with link.exe so load the
       // bin.ld module.
       //
       if (tsys == "win32-msvc")
-      {
-        if (!cast_false<bool> (rs["bin.ld.loaded"]))
-          load_module (rs, rs, "bin.ld", loc);
-      }
+        load_module (rs, rs, "bin.ld", loc);
 
       // If our target is MinGW, then we will need the resource compiler
       // (windres) in order to embed manifests into executables.
       //
       if (tsys == "mingw32")
-      {
-        if (!cast_false<bool> (rs["bin.rc.loaded"]))
-          load_module (rs, rs, "bin.rc", loc);
-      }
+        load_module (rs, rs, "bin.rc", loc);
 
       return true;
     }
@@ -436,13 +417,13 @@ namespace build2
       //
       if (lc && lp && rs["config.c"])
       {
-        load_module (rs, rs, c, loc, false, hints);
-        load_module (rs, rs, cxx, loc, false, hints);
+        init_module (rs, rs, c,   loc, false /* optional */, hints);
+        init_module (rs, rs, cxx, loc, false /* optional */, hints);
       }
       else
       {
-        if (lp) load_module (rs, rs, cxx, loc, false, hints);
-        if (lc) load_module (rs, rs, c, loc, false, hints);
+        if (lp) init_module (rs, rs, cxx, loc, false, hints);
+        if (lc) init_module (rs, rs, c,   loc, false, hints);
       }
 
       return true;
diff --git a/libbuild2/config/operation.cxx b/libbuild2/config/operation.cxx
index 9f241e8..81b99be 100644
--- a/libbuild2/config/operation.cxx
+++ b/libbuild2/config/operation.cxx
@@ -172,7 +172,7 @@ namespace build2
       // saved) and this function can be called from a buildfile (probably
       // only during serial execution but still).
       //
-      module* mod (rs.lookup_module<module> (module::name));
+      module* mod (rs.find_module<module> (module::name));
 
       if (mod == nullptr)
         fail (on) << "no configuration information available during this "
@@ -336,7 +336,7 @@ namespace build2
                 {
                   // Find the config module (might not be there).
                   //
-                  if (auto* m = r->lookup_module<const module> (module::name))
+                  if (auto* m = r->find_module<const module> (module::name))
                   {
                     // Find the corresponding saved module.
                     //
diff --git a/libbuild2/config/utility.cxx b/libbuild2/config/utility.cxx
index 291bf0c..34220f8 100644
--- a/libbuild2/config/utility.cxx
+++ b/libbuild2/config/utility.cxx
@@ -132,14 +132,14 @@ namespace build2
     void
     save_variable (scope& rs, const variable& var, uint64_t flags)
     {
-      if (module* m = rs.lookup_module<module> (module::name))
+      if (module* m = rs.find_module<module> (module::name))
         m->save_variable (var, flags);
     }
 
     void
     save_module (scope& rs, const char* name, int prio)
     {
-      if (module* m = rs.lookup_module<module> (module::name))
+      if (module* m = rs.find_module<module> (module::name))
         m->save_module (name, prio);
     }
 
diff --git a/libbuild2/cxx/init.cxx b/libbuild2/cxx/init.cxx
index 8f404ea..d2601c4 100644
--- a/libbuild2/cxx/init.cxx
+++ b/libbuild2/cxx/init.cxx
@@ -369,8 +369,7 @@ namespace build2
 
       // Load cc.core.vars so that we can cache all the cc.* variables.
       //
-      if (!cast_false<bool> (rs["cc.core.vars.loaded"]))
-        load_module (rs, rs, "cc.core.vars", loc);
+      load_module (rs, rs, "cc.core.vars", loc);
 
       // Enter all the variables and initialize the module data.
       //
@@ -532,10 +531,8 @@ namespace build2
 
       // Load cxx.guess.
       //
-      if (!cast_false<bool> (rs["cxx.guess.loaded"]))
-        load_module (rs, rs, "cxx.guess", loc, false, hints);
+      auto& cm (load_module<config_module> (rs, rs, "cxx.guess", loc, hints));
 
-      config_module& cm (*rs.lookup_module<config_module> ("cxx.guess"));
       cm.init (rs, loc, hints);
       return true;
     }
@@ -580,10 +577,11 @@ namespace build2
 
       // Load cxx.config.
       //
-      if (!cast_false<bool> (rs["cxx.config.loaded"]))
-        load_module (rs, rs, "cxx.config", loc, false, hints);
+      // @@ TODO: move guess to config and use return value?
+      //
+      load_module (rs, rs, "cxx.config", loc, hints);
 
-      config_module& cm (*rs.lookup_module<config_module> ("cxx.guess"));
+      config_module& cm (*rs.find_module<config_module> ("cxx.guess"));
 
       auto& vp (rs.ctx.var_pool.rw (rs));
 
diff --git a/libbuild2/dist/operation.cxx b/libbuild2/dist/operation.cxx
index 12ae464..90be2c0 100644
--- a/libbuild2/dist/operation.cxx
+++ b/libbuild2/dist/operation.cxx
@@ -346,7 +346,7 @@ namespace build2
 
       // Copy over all the files. Apply post-processing callbacks.
       //
-      module& mod (*rs->lookup_module<module> (module::name));
+      module& mod (*rs->find_module<module> (module::name));
 
       prog = prog && show_progress (1 /* max_verb */);
       size_t prog_percent (0);
@@ -380,7 +380,7 @@ namespace build2
             {
               srs = &ctx.scopes.find (out_root / pd);
 
-              if (auto* m = srs->lookup_module<module> (module::name))
+              if (auto* m = srs->find_module<module> (module::name))
                 cbs = &m->callbacks_;
               else
                 fail << "dist module not loaded in subproject " << pd;
diff --git a/libbuild2/file.cxx b/libbuild2/file.cxx
index 8d59d7c..be85745 100644
--- a/libbuild2/file.cxx
+++ b/libbuild2/file.cxx
@@ -1245,7 +1245,7 @@ namespace build2
       module_state& s (p.second);
 
       if (s.boot && s.first)
-        load_module (root, root, p.first, s.loc);
+        init_module (root, root, p.first, s.loc);
     }
 
     for (auto& p: root.root_extra->modules)
@@ -1253,7 +1253,7 @@ namespace build2
       module_state& s (p.second);
 
       if (s.boot && !s.first)
-        load_module (root, root, p.first, s.loc);
+        init_module (root, root, p.first, s.loc);
     }
 
     // Load hooks and root.build.
diff --git a/libbuild2/in/init.cxx b/libbuild2/in/init.cxx
index ece1bcf..95bdb0a 100644
--- a/libbuild2/in/init.cxx
+++ b/libbuild2/in/init.cxx
@@ -84,8 +84,7 @@ namespace build2
 
       // Load in.base.
       //
-      if (!cast_false<bool> (rs["in.base.loaded"]))
-        load_module (rs, rs, "in.base", loc);
+      load_module (rs, rs, "in.base", loc);
 
       // Register rules.
       //
diff --git a/libbuild2/module.cxx b/libbuild2/module.cxx
index eec770b..9bc79ab 100644
--- a/libbuild2/module.cxx
+++ b/libbuild2/module.cxx
@@ -526,7 +526,7 @@ namespace build2
     rs.assign (rs.ctx.var_pool.rw (rs).insert (mod + ".booted")) = true;
   }
 
-  bool
+  module_state*
   init_module (scope& rs,
                scope& bs,
                const string& mod,
@@ -614,6 +614,42 @@ namespace build2
       cv = c;
     }
 
-    return l && c;
+    return l && c ? &i->second : nullptr;
+  }
+
+  // @@ TODO: This is a bit of a fuzzy mess:
+  //
+  //    - The .loaded variable check: it's not clear if init_module()
+  //      already has this semantics?
+  //
+  //    - Why do we use variable instead of the module map entry? Probably
+  //      because of optional. Also entry present if only booted. Need to be
+  //      careful here. Also root vs base!
+  //
+  // Note that it would have been nice to keep these inline but we need the
+  // definition of scope for the variable lookup.
+  //
+  bool
+  load_module (scope& rs,
+               scope& bs,
+               const string& name,
+               const location& loc,
+               bool opt,
+               const variable_map& hints)
+  {
+    return cast_false<bool> (bs[name + ".loaded"]) ||
+      init_module (rs, bs, name, loc, opt, hints);
+  }
+
+  unique_ptr<module_base>&
+  load_module (scope& rs,
+               scope& bs,
+               const string& name,
+               const location& loc,
+               const variable_map& hints)
+  {
+    return cast_false<bool> (bs[name + ".loaded"])
+      ? rs.root_extra->modules.find (name)->second.module
+      : init_module (rs, bs, name, loc, false /* optional */, hints)->module;
   }
 }
diff --git a/libbuild2/module.hxx b/libbuild2/module.hxx
index 34c7245..729cc2e 100644
--- a/libbuild2/module.hxx
+++ b/libbuild2/module.hxx
@@ -96,7 +96,7 @@ namespace build2
   {
     template <typename T>
     T*
-    lookup (const string& name) const
+    find_module (const string& name) const
     {
       auto i (find (name));
       return i != end ()
@@ -111,9 +111,10 @@ namespace build2
   boot_module (scope& root, const string& name, const location&);
 
   // Init the specified module loading its library if necessary. Used by the
-  // parser but also by some modules to init prerequisite modules. Return true
-  // if the module was both successfully loaded and configured (false can only
-  // be returned if optional is true).
+  // parser but also by some modules to init prerequisite modules. Return a
+  // pointer to the corresponding module state if the module was both
+  // successfully loaded and configured and NULL otherwise (which can only
+  // happen if optional is true). Note that the return can be used as a bool.
   //
   // The config_hints variable map can be used to pass configuration hints
   // from one module to another. For example, the cxx modude may pass the
@@ -121,7 +122,7 @@ namespace build2
   // module (which may not always be able to extract the same information from
   // its tools).
   //
-  LIBBUILD2_SYMEXPORT bool
+  LIBBUILD2_SYMEXPORT module_state*
   init_module (scope& root,
                scope& base,
                const string& name,
@@ -129,21 +130,37 @@ namespace build2
                bool optional = false,
                const variable_map& config_hints = empty_variable_map);
 
-  // An alias to use from other modules (we could also distinguish between
-  // boot and init).
+  // A wrapper over init_module() to use from other modules that incorporates
+  // the <name>.loaded variable check (use init_module() directly to sidestep
+  // this check).
   //
-  // @@ TODO: maybe incorporate the .loaded variable check we have all over
-  //          (it's not clear if init_module() already has this semantics)?
+  bool
+  load_module (scope& root,
+               scope& base,
+               const string& name,
+               const location&,
+               bool optional,
+               const variable_map& config_hints = empty_variable_map);
+
+  // As above but always load and return a reference to the module instance
+  // pointer (so it can be moved).
   //
-  inline bool
+  unique_ptr<module_base>&
   load_module (scope& root,
                scope& base,
                const string& name,
-               const location& loc,
-               bool optional = false,
+               const location&,
+               const variable_map& config_hints = empty_variable_map);
+
+  template <typename T>
+  inline T&
+  load_module (scope& root,
+               scope& base,
+               const string& name,
+               const location& l,
                const variable_map& config_hints = empty_variable_map)
   {
-    return init_module (root, base, name, loc, optional, config_hints);
+    return static_cast<T&> (*load_module (root, base, name, l, config_hints));
   }
 
   // Loaded modules (as in libraries).
diff --git a/libbuild2/parser.cxx b/libbuild2/parser.cxx
index 59eeafd..2631845 100644
--- a/libbuild2/parser.cxx
+++ b/libbuild2/parser.cxx
@@ -1905,7 +1905,7 @@ namespace build2
         if (boot_)
           boot_module (*root_, n, l);
         else
-          load_module (*root_, *scope_, n, l, optional);
+          init_module (*root_, *scope_, n, l, optional);
       }
     }
 
diff --git a/libbuild2/version/init.cxx b/libbuild2/version/init.cxx
index 9a56756..3ab3621 100644
--- a/libbuild2/version/init.cxx
+++ b/libbuild2/version/init.cxx
@@ -303,8 +303,7 @@ namespace build2
 
       // Load in.base (in.* variables, in{} target type).
       //
-      if (!cast_false<bool> (rs["in.base.loaded"]))
-        load_module (rs, rs, "in.base", l);
+      load_module (rs, rs, "in.base", l);
 
       module& m (static_cast<module&> (*mod));
       const standard_version& v (m.version);
@@ -312,12 +311,11 @@ namespace build2
       // If the dist module is used, set its dist.package and register the
       // post-processing callback.
       //
-      if (auto* dm = rs.lookup_module<dist::module> (dist::module::name))
+      if (auto* dm = rs.find_module<dist::module> (dist::module::name))
       {
         // Make sure dist is init'ed, not just boot'ed.
         //
-        if (!cast_false<bool> (rs["dist.loaded"]))
-          load_module (rs, rs, "dist", l);
+        load_module (rs, rs, "dist", l);
 
         m.dist_uncommitted = cast_false<bool> (rs["config.dist.uncommitted"]);
 
diff --git a/libbuild2/version/rule.cxx b/libbuild2/version/rule.cxx
index fe999b3..463b6e0 100644
--- a/libbuild2/version/rule.cxx
+++ b/libbuild2/version/rule.cxx
@@ -80,7 +80,7 @@ namespace build2
       // If we match, lookup and cache the module for the update operation.
       //
       if (r && a == perform_update_id)
-        t.data (rs.lookup_module<module> (module::name));
+        t.data (rs.find_module<module> (module::name));
 
       return r;
     }
@@ -319,7 +319,7 @@ namespace build2
       const path& p (t.path ());
 
       const scope& rs (t.root_scope ());
-      const module& m (*rs.lookup_module<module> (module::name));
+      const module& m (*rs.find_module<module> (module::name));
 
       if (!m.rewritten)
         return auto_rmfile (p, false /* active */);