Refactor the macro tables in Buffer.

The use of maps of maps of structs is beyond confusing. Replace that
with a class that hides at least some of the complexity. There is
probably more that could be done along the same lines.

(Note: This fixes a thinko but is otherwise the same as the previous
commit. So it rewrites history.)

(cherry picked from commit dbf24b113ebb559d91b20fd84c7de1b96684d752)

diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index e50fd5c8f20affe84605dde919bcdbcaad4d8593..19012643e0a8d01bce94aca8a1a9ef2c6f523b72 100644 (file)
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -199,23 +199,54 @@ public:
        ///
        mutable TocBackend toc_backend;
 
-       /// macro tables
-       struct MacroDefinition {
-               MacroDefinition() {}
-               MacroDefinition(DocIterator const & s, MacroData const & m)
-                       : scope(s), macro(m) {}
-               // The SCOPE is really just the last position at which the macro
-               // is in force. This will be the end of the file (if there are no
-               // children) unless it is in (say) a note, or an inactive branch,
-               // in which case it will be the end of that construct.
-               DocIterator scope;
-               MacroData macro;
+       class MacroTable {
+       public:
+               struct MacroDefinition {
+                       MacroDefinition() {}
+                       MacroDefinition(DocIterator const & s, MacroData const & m)
+                               : scope(s), macro(m) {}
+                       // The SCOPE is really just the last position at which the macro
+                       // is in force. This will be the end of the file (if there are no
+                       // children) unless it is in (say) a note, or an inactive branch,
+                       // in which case it will be the end of that construct.
+                       DocIterator scope;
+                       MacroData macro;
+               };
+               typedef map<DocIterator, MacroDefinition> MacroDefList;
+               typedef map<docstring, MacroDefList> MacroMap;
+
+               pair<bool, MacroDefList const &> getMacroDefinitions(docstring const & name) const
+               {
+                       MacroMap::const_iterator it = macro_map_.find(name);
+                       if (it == macro_map_.end()) {
+                               static MacroDefList dummy;
+                               return {false, dummy};
+                       }
+                       return {true, it->second};
+               }
+
+               set<docstring> getMacroNames() const
+               {
+                       set<docstring> names;
+                       for (auto const & m : macro_map_)
+                               names.insert(m.first);
+                       return names;
+               }
+
+               void clear() { macro_map_.clear(); }
+               
+               void addMacroDefinition(docstring const & name, 
+                               DocIterator const & pos,
+                               DocIterator const & scope,
+                               MacroData const & data) 
+               {
+                       MacroDefList & m = macro_map_[name];
+                       m[pos] = {scope, data};
+               }
+       private:
+               map<docstring, MacroDefList> macro_map_;
        };
-       typedef map<DocIterator, MacroDefinition> PositionScopeMacroMap;
-       typedef map<docstring, PositionScopeMacroMap> MacroMap;
-       /// map from the macro name to the position map,
-       /// which maps the macro definition position to the scope and the MacroData.
-       MacroMap macros;
+       MacroTable macro_table;
 
        /// Each child Buffer is listed in this map, together with where
        /// it is first included in this Buffer.
@@ -3592,7 +3623,7 @@ typename M::const_iterator greatest_below(M & m, typename M::key_type const & x)
 MacroData const * Buffer::Impl::getBufferMacro(docstring const & name,
                                         DocIterator const & pos) const
 {
-       LYXERR(Debug::MACROS, "Searching for " << to_ascii(name) << " at " << pos);
+       LYXERR(Debug::MACROS, "Searching for `" << to_ascii(name) << "' at " << pos);
 
        // if paragraphs have no macro context set, pos will be empty
        if (pos.empty())
@@ -3603,25 +3634,28 @@ MacroData const * Buffer::Impl::getBufferMacro(docstring const & name,
        MacroData const * bestData = nullptr;
 
        // find macro definitions for name
-       MacroMap::const_iterator nameIt = macros.find(name);
-       if (nameIt != macros.end()) {
+       pair<bool, MacroTable::MacroDefList const &> mdl_pair = 
+               macro_table.getMacroDefinitions(name);
+       if (mdl_pair.first) {
+               MacroTable::MacroDefList const & mdl = mdl_pair.second;
                // find last definition in front of pos
-               PositionScopeMacroMap::const_iterator it
-                       = greatest_below(nameIt->second, pos);
-               if (it != nameIt->second.end()) {
+               MacroTable::MacroDefList::const_iterator it = 
+                               greatest_below(mdl, pos);
+               if (it != mdl.end()) {
                        while (true) {
                                // scope ends behind pos?
                                if (pos < it->second.scope) {
                                        // Looks good, remember this. If there
                                        // is no external macro behind this,
+                                       // (i.e., one in a child file), then
                                        // we found the right one already.
                                        bestPos = it->first;
                                        bestData = &it->second.macro;
                                        break;
                                }
-
+       
                                // try previous macro if there is one
-                               if (it == nameIt->second.begin())
+                               if (it == mdl.begin())
                                        break;
                                --it;
                        }
@@ -3808,10 +3842,8 @@ void Buffer::Impl::updateMacros(DocIterator & it, DocIterator & scope)
                                continue;
 
                        // register macro
-                       // FIXME (Abdel), I don't understand why we pass 'it' here
-                       // instead of 'macroTemplate' defined above... is this correct?
-                       macros[macroTemplate.name()][it] =
-                               Impl::MacroDefinition(scope, MacroData(const_cast<Buffer *>(owner_), it));
+                       macro_table.addMacroDefinition(macroTemplate.name(), it, scope, 
+                               MacroData(const_cast<Buffer *>(owner_), it));
                }
 
                // next paragraph
@@ -3842,7 +3874,7 @@ void Buffer::updateMacros() const
 #endif
        
        // start with empty table
-       d->macros.clear();
+       d->macro_table.clear();
        d->children_positions.clear();
        d->position_to_children.clear();
 
@@ -3917,9 +3949,8 @@ void Buffer::listMacroNames(MacroNameSet & macros) const
 
        d->macro_lock = true;
 
-       // loop over macro names
-       for (auto const & nameit : d->macros)
-               macros.insert(nameit.first);
+       set<docstring> names = d->macro_table.getMacroNames();
+       macros.insert(names.begin(), names.end());
 
        // loop over children
        for (auto const & p : d->children_positions) {