Minor config variable lookup cleanups

8 files changed, 69 insertions, 57 deletions

diff --git a/build2/cli/init.cxx b/build2/cli/init.cxx
index f3f5840..b196351 100644
--- a/build2/cli/init.cxx
+++ b/build2/cli/init.cxx
@@ -281,9 +281,9 @@ namespace build2
       {
         // config.cli.options
         //
-        // This one is optional. We also merge it into the corresponding
-        // cli.* variables. See the cxx module for more information on
-        // this merging semantics and some of its tricky aspects.
+        // This one is optional. We also merge it into the corresponding cli.*
+        // variables. See the cc module for more information on this merging
+        // semantics and some of its tricky aspects.
         //
         bs.assign ("cli.options") += cast_null<strings> (
           config::optional (rs, "config.cli.options"));
diff --git a/libbuild2/bin/init.cxx b/libbuild2/bin/init.cxx
index f6faab5..bb935de 100644
--- a/libbuild2/bin/init.cxx
+++ b/libbuild2/bin/init.cxx
@@ -625,7 +625,7 @@ namespace build2
             "config.bin.ar",
             path (apply_pattern (ar_d, pat.pattern)),
             false,
-            config::save_commented));
+            config::save_default_commented));
 
         auto rp (
           config::required (
@@ -633,7 +633,7 @@ namespace build2
             "config.bin.ranlib",
             nullptr,
             false,
-            config::save_commented));
+            config::save_default_commented));
 
         const path& ar (cast<path> (ap.first));
         const path* ranlib (cast_null<path> (rp.first));
@@ -771,7 +771,7 @@ namespace build2
             "config.bin.ld",
             path (apply_pattern (ld_d, pat.pattern)),
             false,
-            config::save_commented));
+            config::save_default_commented));
 
         const path& ld (cast<path> (p.first));
         ld_info ldi (guess_ld (ld, pat.paths));
@@ -904,7 +904,7 @@ namespace build2
             "config.bin.rc",
             path (apply_pattern (rc_d, pat.pattern)),
             false,
-            config::save_commented));
+            config::save_default_commented));
 
         const path& rc (cast<path> (p.first));
         rc_info rci (guess_rc (rc, pat.paths));
diff --git a/libbuild2/cc/module.cxx b/libbuild2/cc/module.cxx
index d8365e4..138a11b 100644
--- a/libbuild2/cc/module.cxx
+++ b/libbuild2/cc/module.cxx
@@ -113,11 +113,12 @@ namespace build2
           // user changes the source of the pattern/mode, this one will get
           // updated as well.
           //
-          p = config::required (rs,
-                                config_x,
-                                move (d),
-                                false,
-                                cc_loaded ? config::save_commented : 0);
+          p = config::required (
+            rs,
+            config_x,
+            move (d),
+            false,
+            cc_loaded ? config::save_default_commented : 0);
         }
 
         // Split the value into the compiler path and mode.
@@ -349,6 +350,16 @@ namespace build2
       // over. So what we are going to do is only append to a value if
       // it came from this scope. Then the usage for merging becomes:
       //
+      // @@ There are actually two cases to this issue:
+      //
+      //    1. The module is loaded in the outer project (e.g., tests inside a
+      //       project). It feels like this should be handled with project-
+      //       specific variable visibility.
+      //
+      //    2. The module is loaded in the outer scope within the same
+      //       project. We are currently thinking whether we should even
+      //       support loading of modules in non-root scopes.
+      //
       // x.coptions = <overridable options> # Note: '='.
       // using x
       // x.coptions += <overriding options> # Note: '+='.
diff --git a/libbuild2/config/init.cxx b/libbuild2/config/init.cxx
index 57b6290..8dd4d8b 100644
--- a/libbuild2/config/init.cxx
+++ b/libbuild2/config/init.cxx
@@ -120,7 +120,7 @@ namespace build2
         m.save_module ("config", INT32_MIN);
         m.save_module ("import", INT32_MIN);
 
-        m.save_variable (c_p, omit_null);
+        m.save_variable (c_p, save_null_omitted);
       }
 
       // Register the config function family if this is the first instance of
diff --git a/libbuild2/config/operation.cxx b/libbuild2/config/operation.cxx
index 3758c87..2138986 100644
--- a/libbuild2/config/operation.cxx
+++ b/libbuild2/config/operation.cxx
@@ -306,7 +306,7 @@ namespace build2
             // inherited. We might also not have any value at all (see
             // unconfigured()).
             //
-            if (!l.defined () || (l->null && sv.flags & omit_null))
+            if (!l.defined () || (l->null && sv.flags & save_null_omitted))
               continue;
 
             // Handle inherited from outer scope values.
@@ -449,11 +449,11 @@ namespace build2
               first = false;
             }
 
-            // Handle the save_commented flag.
+            // Handle the save_default_commented flag.
             //
             if ((org.first.defined () && org.first->extra) && // Default value.
                 org.first == ovr.first &&                     // Not overriden.
-                (sv.flags & save_commented) == save_commented)
+                (sv.flags & save_default_commented) != 0)
             {
               os << '#' << n << " =" << endl;
               continue;
diff --git a/libbuild2/config/utility.cxx b/libbuild2/config/utility.cxx
index 98047b7..a39d3e0 100644
--- a/libbuild2/config/utility.cxx
+++ b/libbuild2/config/utility.cxx
@@ -19,8 +19,7 @@ namespace build2
     pair<lookup, bool>
     omitted (scope& rs, const variable& var)
     {
-      // This is a stripped-down version of the required() twisted
-      // implementation.
+      // This is a stripped-down version of the required()'s twisted logic.
 
       pair<lookup, size_t> org (rs.find_original (var));
 
diff --git a/libbuild2/config/utility.hxx b/libbuild2/config/utility.hxx
index 6890574..3e01cd1 100644
--- a/libbuild2/config/utility.hxx
+++ b/libbuild2/config/utility.hxx
@@ -39,38 +39,37 @@ namespace build2
     //
     template <typename T>
     pair<lookup, bool>
-    required (scope& root,
+    required (scope& rs,
               const variable&,
               T&& default_value,
               bool override = false,
               uint64_t save_flags = 0);
 
-    // Note that the variable is expected to have already been registered.
+    // Note that the variable is expected to have already been entered.
     //
     template <typename T>
     inline pair<lookup, bool>
-    required (scope& root,
-              const string& name,
+    required (scope& rs,
+              const string& var,
               T&& default_value,
               bool override = false,
               uint64_t save_flags = 0)
     {
-      return required (root,
-                       root.ctx.var_pool[name],
+      return required (rs,
+                       rs.ctx.var_pool[var],
                        std::forward<T> (default_value), // VC14
                        override,
                        save_flags);
     }
 
     inline pair<lookup, bool>
-    required (scope& root,
-              const string& name,
+    required (scope& rs,
+              const string& var,
               const char* default_value,
               bool override = false,
               uint64_t save_flags = 0)
     {
-      return required (
-        root, name, string (default_value), override, save_flags);
+      return required (rs, var, string (default_value), override, save_flags);
     }
 
     // As above, but leave the unspecified value as undefined rather than
@@ -80,14 +79,14 @@ namespace build2
     // out some fallback. See config.bin.target for an example.
     //
     LIBBUILD2_SYMEXPORT pair<lookup, bool>
-    omitted (scope& root, const variable&);
+    omitted (scope& rs, const variable&);
 
-    // Note that the variable is expected to have already been registered.
+    // Note that the variable is expected to have already been entered.
     //
     inline pair<lookup, bool>
-    omitted (scope& root, const string& name)
+    omitted (scope& rs, const string& var)
     {
-      return omitted (root, root.ctx.var_pool[name]);
+      return omitted (rs, rs.ctx.var_pool[var]);
     }
 
     // Set, if necessary, an optional config.* variable. In particular, an
@@ -98,15 +97,18 @@ namespace build2
     //
     // @@ Rename since clashes with the optional class template.
     //
+    // @@ Does it make sense to return the new indicator here as well,
+    //    for consistency/generality.
+    //
     LIBBUILD2_SYMEXPORT lookup
-    optional (scope& root, const variable&);
+    optional (scope& rs, const variable&);
 
     // Note that the variable is expected to have already been registered.
     //
     inline lookup
-    optional (scope& root, const string& name)
+    optional (scope& rs, const string& var)
     {
-      return optional (root, root.ctx.var_pool[name]);
+      return optional (rs, rs.ctx.var_pool[var]);
     }
 
     // Check whether there are any variables specified from the config
@@ -122,35 +124,35 @@ namespace build2
     // running the tests, etc).
     //
     LIBBUILD2_SYMEXPORT bool
-    specified (scope& root, const string& name);
+    specified (scope& rs, const string& var);
 
     // Check if there is a false config.*.configured value. This mechanism can
     // be used to "remember" that the module is left unconfigured in order to
     // avoid re-running the tests, etc.
     //
     LIBBUILD2_SYMEXPORT bool
-    unconfigured (scope& root, const string& name);
+    unconfigured (scope& rs, const string& var);
 
     // Set the config.*.configured value. Note that you only need to set it to
     // false. It will be automatically ignored if there are any other config.*
     // values for this module. Return true if this sets a new value.
     //
     LIBBUILD2_SYMEXPORT bool
-    unconfigured (scope& root, const string& name, bool);
+    unconfigured (scope& rs, const string& var, bool value);
 
     // Enter the variable so that it is saved during configuration. See
     // config::module for details.
     //
-    const uint64_t save_commented = 0x01; // Save default value as commented.
-    const uint64_t omit_null      = 0x02; // Treat NULL as undefined.
+    const uint64_t save_default_commented = 0x01; // Based on value::extra.
+    const uint64_t save_null_omitted      = 0x02; // Treat NULL as undefined.
 
     LIBBUILD2_SYMEXPORT void
-    save_variable (scope& root, const variable&, uint64_t flags = 0);
+    save_variable (scope& rs, const variable&, uint64_t flags = 0);
 
     // Establish module order/priority. See config::module for details.
     //
     LIBBUILD2_SYMEXPORT void
-    save_module (scope& root, const char* name, int prio = 0);
+    save_module (scope& rs, const char* module, int prio = 0);
 
     // Create a project in the specified directory.
     //
@@ -167,11 +169,11 @@ namespace build2
                     uint16_t verbosity = 1);        // Diagnostic verbosity.
 
     inline path
-    config_file (const scope& root)
+    config_file (const scope& rs)
     {
-      return (root.out_path () /
-              root.root_extra->build_dir /
-              "config." + root.root_extra->build_ext);
+      return (rs.out_path () /
+              rs.root_extra->build_dir /
+              "config." + rs.root_extra->build_ext);
     }
   }
 }
diff --git a/libbuild2/config/utility.txx b/libbuild2/config/utility.txx
index b5d2174..670a701 100644
--- a/libbuild2/config/utility.txx
+++ b/libbuild2/config/utility.txx
@@ -10,17 +10,17 @@ namespace build2
   {
     template <typename T>
     pair<lookup, bool>
-    required (scope& root,
+    required (scope& rs,
               const variable& var,
               T&& def_val,
               bool def_ovr,
-              uint64_t save_flags)
+              uint64_t sflags)
     {
       // Note: see also omitted() if changing anything here.
 
-      save_variable (root, var, save_flags);
+      save_variable (rs, var, sflags);
 
-      pair<lookup, size_t> org (root.find_original (var));
+      pair<lookup, size_t> org (rs.find_original (var));
 
       bool n (false); // New flag.
       lookup l (org.first);
@@ -31,23 +31,23 @@ namespace build2
       // are going to do is first ignore overrides and perform the normal
       // logic on the original. Then we apply the overrides on the result.
       //
-      if (!l.defined () || (def_ovr && !l.belongs (root)))
+      if (!l.defined () || (def_ovr && !l.belongs (rs)))
       {
-        value& v (root.assign (var) = std::forward<T> (def_val)); // VC14
+        value& v (rs.assign (var) = std::forward<T> (def_val)); // VC14
         v.extra = true; // Default value flag.
 
-        n = (save_flags & save_commented) == 0; // Absence means default.
-        l = lookup (v, var, root);
-        org = make_pair (l, 1); // Lookup depth is 1 since it's in root.vars.
+        n = (sflags & save_default_commented) == 0; // Absence means default.
+        l = lookup (v, var, rs);
+        org = make_pair (l, 1); // Lookup depth is 1 since it's in rs.vars.
       }
       // Treat an inherited value that was set to default as new.
       //
       else if (l->extra)
-        n = (save_flags & save_commented) == 0; // Absence means default.
+        n = (sflags & save_default_commented) == 0; // Absence means default.
 
       if (var.overrides != nullptr)
       {
-        pair<lookup, size_t> ovr (root.find_override (var, move (org)));
+        pair<lookup, size_t> ovr (rs.find_override (var, move (org)));
 
         if (l != ovr.first) // Overriden?
         {