[PATCH v2] abg-reader.cc: track WIP types by pointer not name
Matthias Maennich
maennich@google.com
Mon Jun 22 20:06:30 GMT 2020
On Mon, Jun 22, 2020 at 06:25:04PM +0100, Giuliano Procida wrote:
>When reading ABI XML files, the reader needs to construct types
>progressively as any type may depend on other types and even on
>itself. Such work-in-progress types are tracked explicitly.
>
>The storage used for this is a map from (external, qualified) type
>name to a count of how many times the type (name) has been seen.
>
>However, function type names are invariably stored as "void ()" as
>they are incomplete at the point they are added to the map. When the
>reader later attempts to remove the marking they have their proper,
>different names. In short, the code doesn't do what it's supposed to.
>
>This commit changes the stored value from string to const type_base*.
>Equality on type_base_sptr has been defined to be a deep comparison so
>storing those wouldn't be quite right.
>
>It also replaces the unordered_map with a simple vector, implementing
>a searchable stack. This is simpler and fast, for the expected number
>of types, and allows a stronger invariant to be asserted due to the
>preservation of insertion order.
>
>Note that this commit removes some of the call paths that result in
>incorrect (external) type names being cached, regardless of whether
>they are actually used.
>
> * src/abg-reader.cc (xml_reader::m_wip_types_map): Replace
> with m_wip_types_stack of type vector<const type_base*>.
> (xml_reader::clear_wip_classes_map): Remove.
> (xml_reader::clear_wip_types_stack): New function, clears
> m_wip_types_stack.
> (xml_reader::mark_type_as_wip): Push type pointer onto
> m_wip_types_stack.
> (xml_reader::unmark_type_as_wip): Add assertion that type
> pointer is at top of stack. Pop pointer off m_wip_types_stack.
> (xml_reader::is_wip_type): Test if type pointer in stack.
>
The surroundings of this code look like a bit of RAII could not harm.
Especially given the invariant that unmarking pops the stack, marking
could probably also define the unmarking by using something like a scope
guard.
Reviewed-by: Matthias Maennich <maennich@google.com>
Cheers,
Matthias
>Signed-off-by: Giuliano Procida <gprocida@google.com>
>---
> src/abg-reader.cc | 33 ++++++++++-----------------------
> 1 file changed, 10 insertions(+), 23 deletions(-)
>
>diff --git a/src/abg-reader.cc b/src/abg-reader.cc
>index eb74659f..480d1402 100644
>--- a/src/abg-reader.cc
>+++ b/src/abg-reader.cc
>@@ -26,6 +26,7 @@
> /// native XML format is named "abixml".
>
> #include "config.h"
>+#include <algorithm>
> #include <cstring>
> #include <cstdlib>
> #include <cerrno>
>@@ -119,7 +120,7 @@ private:
> unordered_map<string, vector<type_base_sptr> > m_types_map;
> unordered_map<string, shared_ptr<function_tdecl> > m_fn_tmpl_map;
> unordered_map<string, shared_ptr<class_tdecl> > m_class_tmpl_map;
>- unordered_map<string, size_t> m_wip_types_map;
>+ vector<const type_base*> m_wip_types_stack;
> vector<type_base_sptr> m_types_to_canonicalize;
> string_xml_node_map m_id_xml_node_map;
> xml_node_decl_base_sptr_map m_xml_node_decl_map;
>@@ -522,8 +523,8 @@ public:
> /// the map of the class that are currently being built, but at not
> /// yet fully built.
> void
>- clear_wip_classes_map()
>- {m_wip_types_map.clear();}
>+ clear_wip_types_stack()
>+ {m_wip_types_stack.clear();}
>
> /// Mark a given type as being "Work In Progress"; that is, mark it
> /// as being currently built.
>@@ -534,12 +535,7 @@ public:
> {
> if (!t)
> return;
>- string qname = get_type_name(t, /*qualified=*/true);
>- unordered_map<string, size_t>::iterator it = m_wip_types_map.find(qname);
>- if (it == m_wip_types_map.end())
>- m_wip_types_map[qname] = 1;
>- else
>- ++it->second;
>+ m_wip_types_stack.push_back(t.get());
> }
>
> /// Mark a given class as being *NOT* "Work In Progress" anymore;
>@@ -551,15 +547,9 @@ public:
> {
> if (!t)
> return;
>-
>- string qname = get_type_name(t, /*qualified=*/true);
>- unordered_map<string, size_t>::iterator it = m_wip_types_map.find(qname);
>- if (it == m_wip_types_map.end())
>- return;
>- if (it->second)
>- --it->second;
>- if (it->second == 0)
>- m_wip_types_map.erase(it);
>+ ABG_ASSERT(!m_wip_types_stack.empty());
>+ ABG_ASSERT(m_wip_types_stack.back() == t.get());
>+ m_wip_types_stack.pop_back();
> }
>
> /// Test if a type is being currently built; that is, if it's "Work
>@@ -571,11 +561,8 @@ public:
> {
> if (!t)
> return false;
>-
>- string qname = get_type_name(t, /*qualified=*/true);
>- unordered_map<string, size_t>::const_iterator i =
>- m_wip_types_map.find(qname);
>- return i != m_wip_types_map.end();
>+ return std::find(m_wip_types_stack.begin(), m_wip_types_stack.end(), t.get())
>+ != m_wip_types_stack.end();
> }
>
> /// Test if two types are equal, without comparing them structurally.
>--
>2.27.0.111.gc72c7da667-goog
>
More information about the Libabigail
mailing list