From: Richard Kimberly Heck <rikiheck@lyx.org>
Date: Mon, 9 Nov 2020 23:29:26 +0000 (-0500)
Subject: Refactor the macro tables in Buffer.
X-Git-Url: https://git.lyx.org/gitweb/?a=commitdiff_plain;h=10c39c879e99d1263faf56ec03505f3d4fce532f;p=features.git

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 e50fd5c8f2..19012643e0 100644
--- 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) {