Save config vars in order specified rather than alphabetically

This way we can group them semantically which results in easier to
understand config.build output.

5 files changed, 205 insertions, 173 deletions

diff --git a/build2/config/module b/build2/config/module
index aed85a5..5becfd4 100644
--- a/build2/config/module
+++ b/build2/config/module
@@ -17,15 +17,39 @@ namespace build2
 {
   namespace config
   {
-    struct module: module_base
+    // An ordered list of modules each with an ordered list of list of
+    // config.* variables and their "save flags" (see save_variable()) that
+    // are used (as opposed to just being specified) in this configuration.
+    // Populated by the config utility functions (required(), optional())
+    // and saved in the order populated.
+    //
+    struct saved_variable
+    {
+      reference_wrapper<const variable> var;
+      uint64_t flags;
+    };
+
+    using saved_variables = vector<saved_variable>;
+
+    struct saved_modules: butl::prefix_map<string, saved_variables, '.'>
     {
-      // A sorted list of config.* variables and their "save flags" (see
-      // save_variable()) that are used (as opposed to just being specified)
-      // in this configuration. Populated by the config utility functions
-      // (required(), optional())
-      //
-      butl::prefix_map<variable_cref, uint64_t, '.'> vars;
+      vector<const_iterator> sequence;
+
+      iterator
+      insert (string name)
+      {
+        auto p (emplace (move (name), saved_variables ()));
 
+        if (p.second)
+          sequence.push_back (p.first);
+
+        return p.first;
+      }
+    };
+
+    struct module: module_base
+    {
+      config::saved_modules saved_modules;
       static const string name;
     };
 
diff --git a/build2/config/operation.cxx b/build2/config/operation.cxx
index 767ca1a..1b40832 100644
--- a/build2/config/operation.cxx
+++ b/build2/config/operation.cxx
@@ -87,192 +87,166 @@ namespace build2
 
         ofs << "# Created automatically by the config module, but feel " <<
           "free to edit." << endl
-            << "#" << endl
-            << endl;
+            << "#" << endl;
 
         if (auto l = root.vars["amalgamation"])
         {
           const dir_path& d (cast<dir_path> (l));
 
-          ofs << "# Base configuration inherited from " << d << endl
-              << "#" << endl
-              << endl;
+          ofs << endl
+              << "# Base configuration inherited from " << d << endl
+              << "#" << endl;
         }
 
-        // Separate variables for modules with blank lines.
-        //
-        const string* mod_s (nullptr);
-        size_t mod_n (0);
-
-        auto next_module = [&mod_s, &mod_n] (const variable& var) -> bool
-        {
-          const string& s (var.name);
-
-          size_t p (s.find ('.', 7)); // 7 for "config."
-          size_t n (p != string::npos ? p - 7 : s.size () - 7);
-
-          if (mod_s == nullptr)
-          {
-            mod_s = &s;
-            mod_n = n;
-            return false; // First
-          }
-
-          if (s.compare (7, n, *mod_s, 7, mod_n) != 0)
-          {
-            mod_s = &s;
-            mod_n = n;
-            return true; // Next.
-          }
-
-          return false;
-        };
-
         // Save config variables.
         //
         names storage;
 
-        for (auto b (mod.vars.begin ()), i (b), e (mod.vars.end ());
-             i != e;
-             ++i)
+        for (const saved_modules::const_iterator& i:
+               mod.saved_modules.sequence)
         {
-          const auto& p (*i);
-          const variable& var (p.first);
-          uint64_t sflags (p.second);
-
-          pair<lookup, size_t> org (root.find_original (var));
-          pair<lookup, size_t> ovr (var.override == nullptr
-                                    ? org
-                                    : root.find_override (var, org));
-          const lookup& l (ovr.first);
-
-          // We definitely write values that are set on our root scope or are
-          // global overrides. Anything in-between is presumably inherited.
-          // We might also not have any value at all (see unconfigured()).
+          const string& sname (i->first);
+          const saved_variables& svars (i->second);
+
+          // Separate modules with a blank line.
           //
-          if (!l.defined ())
-            continue;
+          ofs << endl;
 
-          if (!(l.belongs (root) || l.belongs (*global_scope)))
+          for (const saved_variable& sv: svars)
           {
-            // This is presumably an inherited value. But it could also be
-            // some left-over garbage. For example, our amalgamation could
-            // have used a module but then dropped it while its configuration
-            // values are still lingering in config.build. They are probably
-            // still valid and we should probably continue using them but we
-            // definitely want to move them to our config.build since they
-            // will be dropped from the amalgamation's config.build. Let's
-            // also warn the user just in case.
+            const variable& var (sv.var);
+
+            pair<lookup, size_t> org (root.find_original (var));
+            pair<lookup, size_t> ovr (var.override == nullptr
+                                      ? org
+                                      : root.find_override (var, org));
+            const lookup& l (ovr.first);
+
+            // We definitely write values that are set on our root scope or
+            // are global overrides. Anything in-between is presumably
+            // inherited. We might also not have any value at all (see
+            // unconfigured()).
             //
-            bool found (false);
-            scope* r (&root);
-            while ((r = r->parent_scope ()->root_scope ()) != nullptr)
+            if (!l.defined ())
+              continue;
+
+            if (!(l.belongs (root) || l.belongs (*global_scope)))
             {
-              if (l.belongs (*r))
+              // This is presumably an inherited value. But it could also be
+              // some left-over garbage. For example, an amalgamation could
+              // have used a module but then dropped it while its config
+              // values are still lingering in config.build. They are probably
+              // still valid and we should probably continue using them but we
+              // definitely want to move them to our config.build since they
+              // will be dropped from the amalgamation's config.build. Let's
+              // also warn the user just in case.
+              //
+              bool found (false);
+              scope* r (&root);
+              while ((r = r->parent_scope ()->root_scope ()) != nullptr)
               {
-                if (auto* m = r->modules.lookup<const module> (module::name))
-                  found = m->vars.find (var) != m->vars.end ();
-
-                break;
+                if (l.belongs (*r))
+                {
+                  // Find config module.
+                  //
+                  if (auto* m = r->modules.lookup<const module> (module::name))
+                  {
+                    // Find the corresponding saved module.
+                    //
+                    auto i (m->saved_modules.find (sname));
+
+                    if (i != m->saved_modules.end ())
+                    {
+                      // Find the variable. For now we do linear search.
+                      //
+                      const saved_variables& sv (i->second);
+                      found = find_if (
+                        sv.begin (),
+                        sv.end (),
+                        [&var] (const saved_variable& v) {
+                          return var == v.var;}) != sv.end ();
+                    }
+                  }
+
+                  break;
+                }
               }
-            }
-
-            if (found) // Inherited.
-              continue;
-
-            location loc (&f);
 
-            // If this value is not defined in a project's root scope, then
-            // something is broken.
-            //
-            if (r == nullptr)
-              fail (loc) << "inherited variable " << var.name << " value "
-                         << "is not from a root scope";
+              if (found) // Inherited.
+                continue;
 
-            // If none of the outer project's configurations use this value,
-            // then we warn and save as our own. One special case where we
-            // don't want to warn the user is if the variable is overriden.
-            //
-            if (org.first == ovr.first)
-            {
-              diag_record dr;
-              dr << warn (loc) << "saving previously inherited variable "
-                 << var.name;
+              location loc (&f);
 
-              dr << info (loc) << "because project " << r->out_path ()
-                 << " no longer uses it in its configuration";
+              // If this value is not defined in a project's root scope, then
+              // something is broken.
+              //
+              if (r == nullptr)
+                fail (loc) << "inherited variable " << var.name << " value "
+                           << "is not from a root scope";
 
-              if (verb >= 2)
+              // If none of the outer project's configurations use this value,
+              // then we warn and save as our own. One special case where we
+              // don't want to warn the user is if the variable is overriden.
+              //
+              if (org.first == ovr.first)
               {
-                dr << info (loc) << "variable value: ";
+                diag_record dr;
+                dr << warn (loc) << "saving previously inherited variable "
+                   << var.name;
 
-                if (*l)
+                dr << info (loc) << "because project " << r->out_path ()
+                   << " no longer uses it in its configuration";
+
+                if (verb >= 2)
                 {
-                  names storage;
-                  dr << "'" << reverse (*l, storage) << "'";
+                  dr << info (loc) << "variable value: ";
+
+                  if (*l)
+                  {
+                    storage.clear ();
+                    dr << "'" << reverse (*l, storage) << "'";
+                  }
+                  else
+                    dr << "[null]";
                 }
-                else
-                  dr << "[null]";
               }
             }
-          }
 
-          const string& n (var.name);
-          const value& v (*l);
+            const string& n (var.name);
+            const value& v (*l);
 
-          // We will only write config.*.configured if it is false (true is
-          // implied by its absence). We will also ignore false values if
-          // there is any other value for this module (see unconfigured()).
-          //
-          if (n.size () > 11 &&
-              n.compare (n.size () - 11, 11, ".configured") == 0)
-          {
-            if (cast<bool> (v))
-              continue;
-
-            size_t m (n.size () - 11); // Prefix size.
-            auto same = [&n, m] (const variable& v)
-            {
-              return v.name.size () >= m &&
-                v.name.compare (0, m, n, 0, m) == 0;
-            };
-
-            // Check if this is the first value for this module.
+            // We will only write config.*.configured if it is false (true is
+            // implied by its absence). We will also ignore false values if
+            // there is any other value for this module (see unconfigured()).
             //
-            auto j (i);
-            if (j != b && same ((--j)->first))
-              continue;
+            if (n.size () > 11 &&
+                n.compare (n.size () - 11, 11, ".configured") == 0)
+            {
+              if (cast<bool> (v) || svars.size () != 1)
+                continue;
+            }
 
-            // Check if this is the last value for this module.
+            // Handle the save_commented flag.
             //
-            j = i;
-            if (++j != e && same (j->first))
+            if ((org.first.defined () && org.first->extra) && // Default value.
+                org.first == ovr.first &&                     // Not overriden.
+                (sv.flags & save_commented) == save_commented)
+            {
+              ofs << '#' << n << " =" << endl;
               continue;
-          }
-
-          if (next_module (var))
-            ofs << endl;
-
-          // Handle the save_commented flag.
-          //
-          if ((org.first.defined () && org.first->extra) && // Default value.
-              org.first == ovr.first &&                     // Not overriden.
-              (sflags & save_commented) == save_commented)
-          {
-            ofs << '#' << n << " =" << endl;
-            continue;
-          }
+            }
 
-          if (v)
-          {
-            storage.clear ();
+            if (v)
+            {
+              storage.clear ();
 
-            ofs << n << " = ";
-            to_stream (ofs, reverse (v, storage), true, '@'); // Quote.
-            ofs << endl;
+              ofs << n << " = ";
+              to_stream (ofs, reverse (v, storage), true, '@'); // Quote.
+              ofs << endl;
+            }
+            else
+              ofs << n << " = [null]" << endl;
           }
-          else
-            ofs << n << " = [null]" << endl;
         }
       }
       catch (const ofstream::failure&)
diff --git a/build2/config/utility.cxx b/build2/config/utility.cxx
index 768a70d..a24f7b3 100644
--- a/build2/config/utility.cxx
+++ b/build2/config/utility.cxx
@@ -14,19 +14,6 @@ namespace build2
 {
   namespace config
   {
-    void
-    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 = r.modules.lookup<module> (module::name))
-          mod->vars.emplace (var, flags);
-      }
-    }
-
     pair<const value*, bool>
     required (scope& r, const variable& var)
     {
@@ -130,5 +117,51 @@ namespace build2
 
       root.assign (var) = !v;
     }
+
+    void
+    save_variable (scope& r, const variable& var, uint64_t flags)
+    {
+      if (current_mif->id != configure_id)
+        return;
+
+      // The project might not be using the config module. But then how
+      // could we be configuring it? Good question.
+      //
+      if (module* m = r.modules.lookup<module> (module::name))
+      {
+        const string& n (var.name);
+
+        // First try to find the module with the name that is the longest
+        // prefix of this variable name.
+        //
+        saved_modules& sm (m->saved_modules);
+        auto i (sm.end ());
+
+        if (!sm.empty ())
+        {
+          i = sm.upper_bound (n);
+
+          // Get the greatest less than, if any. We might still not be a
+          // suffix. And we still have to check the last element if
+          // upper_bound() returned end().
+          //
+          if (i == sm.begin () || !sm.key_comp ().prefix ((--i)->first, n))
+            i = sm.end ();
+        }
+
+        // If no module matched, then create one based on the variable name.
+        //
+        if (i == sm.end ())
+        {
+          // @@ For now with 'config.' prefix.
+          //
+          i = sm.insert (string (n, 0, n.find ('.', 7)));
+        }
+
+        // We assume each variable is saved/configured once.
+        //
+        i->second.push_back (saved_variable {var, flags});
+      }
+    }
   }
 }
diff --git a/build2/cxx/compile.cxx b/build2/cxx/compile.cxx
index 432b485..300fad7 100644
--- a/build2/cxx/compile.cxx
+++ b/build2/cxx/compile.cxx
@@ -851,7 +851,7 @@ namespace build2
             i = pm.upper_bound (d);
 
             // Get the greatest less than, if any. We might still not be a
-            // sub. Note also that we still have to check the last element is
+            // sub. Note also that we still have to check the last element if
             // upper_bound() returned end().
             //
             if (i == pm.begin () || !d.sub ((--i)->first))
diff --git a/build2/variable b/build2/variable
index 08d0020..e2dc943 100644
--- a/build2/variable
+++ b/build2/variable
@@ -119,8 +119,6 @@ namespace build2
   inline bool
   operator== (const variable& x, const variable& y) {return x.name == y.name;}
 
-  typedef reference_wrapper<const variable> variable_cref;
-
   //
   //
   class value
@@ -647,7 +645,8 @@ namespace std
 namespace butl
 {
   template <>
-  struct compare_prefix<build2::variable_cref>: compare_prefix<std::string>
+  struct compare_prefix<std::reference_wrapper<const build2::variable>>:
+    compare_prefix<std::string>
   {
     typedef compare_prefix<std::string> base;
 
@@ -713,7 +712,9 @@ namespace build2
   class variable_map
   {
   public:
-    using map_type = butl::prefix_map<variable_cref, value, '.'>;
+    using map_type = butl::prefix_map<reference_wrapper<const variable>,
+                                      value,
+                                      '.'>;
     using size_type = map_type::size_type;
 
     template <typename I>