[RFC,1/2] Support declaration-only enums.

This is an almost blind attempt at addition of support for incomplete,
also known as forward-declared, enum types. I've not made any attempt
to refactor or share logic with the struct/union code and I've
probably got other things wrong.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 include/abg-comp-filter.h   |   7 +
 include/abg-comparison.h    |   7 +
 include/abg-fwd.h           |  26 ++-
 include/abg-ir.h            |  23 +++
 src/abg-comp-filter.cc      |  78 ++++++++-
 src/abg-comparison.cc       |  21 ++-
 src/abg-default-reporter.cc |  34 +++-
 src/abg-dwarf-reader.cc     | 308 +++++++++++++++++++++++++++++++++++-
 src/abg-ir.cc               | 235 ++++++++++++++++++++++++++-
 9 files changed, 713 insertions(+), 26 deletions(-)

Hi.

On Tue, 12 May 2020 at 15:03, Matthias Maennich <maennich@google.com> wrote:

> On Thu, Apr 23, 2020 at 02:03:55PM +0100, Giuliano Procida wrote:
> >This is an almost blind attempt at addition of support for incomplete,
> >also known as forward-declared, enum types. I've not made any attempt
> >to refactor or share logic with the struct/union code and I've
> >probably got other things wrong.
> >
> >Signed-off-by: Giuliano Procida <gprocida@google.com>
> >---
> > include/abg-comp-filter.h   |   7 +
> > include/abg-comparison.h    |   7 +
> > include/abg-fwd.h           |  26 ++-
> > include/abg-ir.h            |  23 +++
> > src/abg-comp-filter.cc      |  78 ++++++++-
> > src/abg-comparison.cc       |  21 ++-
> > src/abg-default-reporter.cc |  34 +++-
> > src/abg-dwarf-reader.cc     | 308 +++++++++++++++++++++++++++++++++++-
> > src/abg-ir.cc               | 235 ++++++++++++++++++++++++++-
> > 9 files changed, 713 insertions(+), 26 deletions(-)
> >
> >diff --git a/include/abg-comp-filter.h b/include/abg-comp-filter.h
> >index b8da3156..c486fec5 100644
> >--- a/include/abg-comp-filter.h
> >+++ b/include/abg-comp-filter.h
> >@@ -68,9 +68,16 @@ bool
> > has_class_decl_only_def_change(const class_or_union_sptr& first,
> >                              const class_or_union_sptr& second);
> >
> >+bool
> >+has_enum_decl_only_def_change(const enum_type_decl_sptr& first,
> >+                            const enum_type_decl_sptr& second);
> >+
> > bool
> > has_class_decl_only_def_change(const diff *diff);
> >
> >+bool
> >+has_enum_decl_only_def_change(const diff *diff);
> >+
> > bool
> > has_basic_type_name_change(const diff *);
> >
> >diff --git a/include/abg-comparison.h b/include/abg-comparison.h
> >index 4f60ff99..bb654cef 100644
> >--- a/include/abg-comparison.h
> >+++ b/include/abg-comparison.h
> >@@ -430,6 +430,12 @@ enum diff_category
> >   /// array type of a global variable, but the ELF size of the
> >   /// variable didn't change.
> >   BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY = 1 << 19,
> >+
> >+  /// This means that a diff node in the sub-tree carries a enum type
> >+  /// that was declaration-only and that is now defined, or vice
> >+  /// versa.
> >+  ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY = 1 << 20,
> >+
> >   /// A special enumerator that is the logical 'or' all the
> >   /// enumerators above.
> >   ///
> >@@ -456,6 +462,7 @@ enum diff_category
> >   | VAR_TYPE_CV_CHANGE_CATEGORY
> >   | VOID_PTR_TO_PTR_CHANGE_CATEGORY
> >   | BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY
> >+  | ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY
> > }; // enum diff_category
> >
> > diff_category
> >diff --git a/include/abg-fwd.h b/include/abg-fwd.h
> >index 1aab70a6..d1cf0322 100644
> >--- a/include/abg-fwd.h
> >+++ b/include/abg-fwd.h
> >@@ -154,9 +154,15 @@ typedef weak_ptr<typedef_decl> typedef_decl_wptr;
> >
> > class enum_type_decl;
> >
> >-/// Convenience typedef for shared pointer on enum_type_decl.
> >+/// Convenience typedef for shared pointer to a @ref enum_type_decl.
> > typedef shared_ptr<enum_type_decl> enum_type_decl_sptr;
> >
> >+/// Convenience typedef for a vector of @ref enum_type_decl_sptr
> >+typedef vector<enum_type_decl_sptr> enums_type;
> >+
> >+/// Convenience typedef for a weak pointer to a @ref enum_type_decl.
> >+typedef weak_ptr<enum_type_decl> enum_type_decl_wptr;
> >+
> > class class_or_union;
> >
> > typedef shared_ptr<class_or_union> class_or_union_sptr;
> >@@ -422,6 +428,12 @@ is_typedef(const type_base*);
> > typedef_decl*
> > is_typedef(type_base*);
> >
> >+enum_type_decl_sptr
> >+is_compatible_with_enum_type(const type_base_sptr&);
> >+
> >+enum_type_decl_sptr
> >+is_compatible_with_enum_type(const decl_base_sptr&);
> >+
> > enum_type_decl_sptr
> > is_enum_type(const type_or_decl_base_sptr&);
> >
> >@@ -512,6 +524,12 @@ look_through_decl_only_class(const class_or_union&);
> > class_or_union_sptr
> > look_through_decl_only_class(class_or_union_sptr);
> >
> >+enum_type_decl_sptr
> >+look_through_decl_only_enum(const enum_type_decl&);
> >+
> >+enum_type_decl_sptr
> >+look_through_decl_only_enum(enum_type_decl_sptr);
> >+
> > var_decl*
> > is_var_decl(const type_or_decl_base*);
> >
> >@@ -1071,6 +1089,12 @@ lookup_enum_type(const string&, const corpus&);
> > enum_type_decl_sptr
> > lookup_enum_type(const interned_string&, const corpus&);
> >
> >+const type_base_wptrs_type*
> >+lookup_enum_types(const interned_string&, const corpus&);
> >+
> >+const type_base_wptrs_type*
> >+lookup_enum_types(const string&, const corpus&);
> >+
> > enum_type_decl_sptr
> > lookup_enum_type_per_location(const interned_string&, const corpus&);
> >
> >diff --git a/include/abg-ir.h b/include/abg-ir.h
> >index fda10de5..88ce39e0 100644
> >--- a/include/abg-ir.h
> >+++ b/include/abg-ir.h
> >@@ -2465,6 +2465,8 @@ public:
> >                const string&          mangled_name = "",
> >                visibility             vis = VISIBILITY_DEFAULT);
> >
> >+  enum_type_decl(const environment* env, const string& name);
> >+
> >   type_base_sptr
> >   get_underlying_type() const;
> >
> >@@ -2474,6 +2476,27 @@ public:
> >   enumerators&
> >   get_enumerators();
> >
> >+  bool
> >+  get_is_declaration_only() const;
> >+
> >+  void
> >+  set_is_declaration_only(bool f);
> >+
> >+  void
> >+  set_definition_of_declaration(enum_type_decl_sptr);
> >+
> >+  const enum_type_decl_sptr
> >+  get_definition_of_declaration() const;
> >+
> >+  const enum_type_decl*
> >+  get_naked_definition_of_declaration() const;
> >+
> >+  decl_base_sptr
> >+  get_earlier_declaration() const;
> >+
> >+  void
> >+  set_earlier_declaration(decl_base_sptr declaration);
> >+
> >   virtual string
> >   get_pretty_representation(bool internal = false,
> >                           bool qualified_name = true) const;
> >diff --git a/src/abg-comp-filter.cc b/src/abg-comp-filter.cc
> >index 75df901c..c8a031d3 100644
> >--- a/src/abg-comp-filter.cc
> >+++ b/src/abg-comp-filter.cc
> >@@ -118,6 +118,25 @@ there_is_a_decl_only_class(const class_decl_sptr&
> class1,
> >   return false;
> > }
> >
> >+/// Test if there is a enum that is declaration-only among the two
> >+/// enumes in parameter.
> >+///
> >+/// @param enum1 the first enum to consider.
> >+///
> >+/// @param enum2 the second enum to consider.
> >+///
> >+/// @return true if either enumes are declaration-only, false
> >+/// otherwise.
> >+static bool
> >+there_is_a_decl_only_enum(const enum_type_decl_sptr& enum1,
> >+                        const enum_type_decl_sptr& enum2)
> >+{
> >+  if ((enum1 && enum1->get_is_declaration_only())
> >+      || (enum2 && enum2->get_is_declaration_only()))
> >+    return true;
> >+  return false;
> >+}
> >+
> > /// Test if the diff involves a declaration-only class.
> > ///
> > /// @param diff the class diff to consider.
> >@@ -146,7 +165,9 @@ type_size_changed(const type_base_sptr f, const
> type_base_sptr s)
> >       || f->get_size_in_bits() == 0
> >       || s->get_size_in_bits() == 0
> >       || there_is_a_decl_only_class(is_compatible_with_class_type(f),
> >-                                  is_compatible_with_class_type(s)))
> >+                                  is_compatible_with_class_type(s))
> >+      || there_is_a_decl_only_enum(is_compatible_with_enum_type(f),
> >+                                 is_compatible_with_enum_type(s)))
> >     return false;
> >
> >   return f->get_size_in_bits() != s->get_size_in_bits();
> >@@ -937,13 +958,38 @@ has_class_decl_only_def_change(const
> class_or_union_sptr& first,
> >   return (f->get_is_declaration_only() != s->get_is_declaration_only());
> > }
> >
> >+/// Test if two @ref enum_sptr are different just by the
> >+/// fact that one is decl-only and the other one is defined.
> >+///
> >+/// @param first the first enum to consider.
> >+///
> >+/// @param second the second enum to consider.
> >+///
> >+/// @return true iff the two arguments are different just by the fact
> >+/// that one is decl-only and the other one is defined.
> >+bool
> >+has_enum_decl_only_def_change(const enum_type_decl_sptr& first,
> >+                            const enum_type_decl_sptr& second)
> >+{
> >+  if (!first || !second)
> >+    return false;
> >+
> >+  enum_type_decl_sptr f = look_through_decl_only_enum(first);
> >+  enum_type_decl_sptr s = look_through_decl_only_enum(second);
> >+
> >+  if (f->get_qualified_name() != s->get_qualified_name())
> >+    return false;
> >+
> >+  return (f->get_is_declaration_only() != s->get_is_declaration_only());
> >+}
> >+
> > /// Test if a class_or_union_diff carries a change in which the two
> > /// classes are different by the fact that one is a decl-only and the
> > /// other one is defined.
> > ///
> > /// @param diff the diff node to consider.
> >-////
> >-//// @return true if the class_or_union_diff carries a change in which
> >+///
> >+/// @return true if the class_or_union_diff carries a change in which
> > /// the two classes are different by the fact that one is a decl-only
> > /// and the other one is defined.
> > bool
> >@@ -961,6 +1007,28 @@ has_class_decl_only_def_change(const diff *diff)
> >   return has_class_decl_only_def_change(f, s);
> > }
> >
> >+/// Test if a enum_diff carries a change in which the two
> >+/// enumes are different by the fact that one is a decl-only and the
> >+/// other one is defined.
> >+///
> >+/// @param diff the diff node to consider.
> >+///
> >+/// @return true if the enum_diff carries a change in which
> >+/// the two enumes are different by the fact that one is a decl-only
> >+/// and the other one is defined.
> >+bool
> >+has_enum_decl_only_def_change(const diff *diff)
> >+{
> >+  const enum_diff *d = dynamic_cast<const enum_diff*>(diff);
> >+  if (!d)
> >+    return false;
> >+
> >+  enum_type_decl_sptr f = look_through_decl_only_enum(d->first_enum());
> >+  enum_type_decl_sptr s = look_through_decl_only_enum(d->second_enum());
> >+
> >+  return has_enum_decl_only_def_change(f, s);
> >+}
> >+
> > /// Test if a diff node carries a basic type name change.
> > ///
> > /// @param d the diff node to consider.
> >@@ -1501,6 +1569,9 @@ categorize_harmless_diff_node(diff *d, bool pre)
> >       if (has_class_decl_only_def_change(d))
> >       category |= CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY;
> >
> >+      if (has_enum_decl_only_def_change(d))
> >+      category |= ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY;
> >+
> >       if (access_changed(f, s))
> >       category |= ACCESS_CHANGE_CATEGORY;
> >
> >@@ -1586,6 +1657,7 @@ categorize_harmful_diff_node(diff *d, bool pre)
> >       //
> >       // TODO: be more specific -- not all size changes are harmful.
> >       if (!has_class_decl_only_def_change(d)
> >+        && !has_enum_decl_only_def_change(d)
> >         && (type_size_changed(f, s)
> >             || data_member_offset_changed(f, s)
> >             || non_static_data_member_type_size_changed(f, s)
> >diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
> >index 46bf9e30..40d80073 100644
> >--- a/src/abg-comparison.cc
> >+++ b/src/abg-comparison.cc
> >@@ -2952,7 +2952,8 @@ get_default_harmless_categories_bitmap()
> >         | abigail::comparison::FN_RETURN_TYPE_CV_CHANGE_CATEGORY
> >         | abigail::comparison::VAR_TYPE_CV_CHANGE_CATEGORY
> >         | abigail::comparison::VOID_PTR_TO_PTR_CHANGE_CATEGORY
> >-        | abigail::comparison::BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY);
> >+        | abigail::comparison::BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY
> >+        | abigail::comparison::ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY);
>
> I would move that up to the line with
>
>    | abigail::comparison::CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY
>

Actually, I think it should just be a single TYPE_DECL_ONLY... or even
DECL_ONLY... CLASS already includes structs and (possibly, the support may
be incomplete) unions.

> }
> >
> > /// Getter of a bitmap made of the set of change categories that are
> >@@ -3113,7 +3114,7 @@ operator<<(ostream& o, diff_category c)
> >       emitted_a_category |= true;
> >     }
> >
> >-    if (c & FN_RETURN_TYPE_CV_CHANGE_CATEGORY)
> >+  if (c & FN_RETURN_TYPE_CV_CHANGE_CATEGORY)
>
> Correct, but unrelated.
>

Ack, moved into another commit.


> >     {
> >       if (emitted_a_category)
> >       o << "|";
> >@@ -3121,7 +3122,7 @@ operator<<(ostream& o, diff_category c)
> >       emitted_a_category |= true;
> >     }
> >
> >-   if (c & VAR_TYPE_CV_CHANGE_CATEGORY)
> >+  if (c & VAR_TYPE_CV_CHANGE_CATEGORY)
> >     {
> >       if (emitted_a_category)
> >       o << "|";
> >@@ -3129,7 +3130,7 @@ operator<<(ostream& o, diff_category c)
> >       emitted_a_category |= true;
> >     }
> >
> >-    if (c & VOID_PTR_TO_PTR_CHANGE_CATEGORY)
> >+  if (c & VOID_PTR_TO_PTR_CHANGE_CATEGORY)
> >     {
> >       if (emitted_a_category)
> >       o << "|";
> >@@ -3137,7 +3138,7 @@ operator<<(ostream& o, diff_category c)
> >       emitted_a_category |= true;
> >     }
> >
> >-    if (c & BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY)
> >+  if (c & BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY)
> >     {
> >       if (emitted_a_category)
> >       o << "|";
> >@@ -3145,6 +3146,14 @@ operator<<(ostream& o, diff_category c)
> >       emitted_a_category |= true;
> >     }
> >
> >+  if (c & ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY)
> >+    {
> >+      if (emitted_a_category)
> >+      o << "|";
> >+      o << "ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY";
> >+      emitted_a_category |= true;
> >+    }
> >+
> >   return o;
> > }
> >
> >@@ -10707,6 +10716,8 @@ struct leaf_diff_node_marker_visitor : public
> diff_node_visitor
> >       && !is_anonymous_class_or_union_diff(d)
> >       // Don't show decl-only-ness changes of classes either.
> >       && !filtering::has_class_decl_only_def_change(d)
> >+      // Same for enums.
>
> The comment should be self-containing.
>

Similar to the comment about a single category, this should really just be
"has_decl_only_def_change".

Here things get a little harder as all the declaration-only pieces are at
the wrong level of the inheritance hierarchy to allow this to work in the
obvious way. We can side-step this for the moment and hide the complexity
in one place until such time as the hierarchy is reworked.

More words: class_or_union, enum_type_decl, base_decl, base_type,
scoped_type_decl. These are all part of the exposed API.


> >+      && !filtering::has_enum_decl_only_def_change(d)
> >       // Sometime, we can encounter artifacts of bogus DWARF that
> >       // yield a diff node for a decl-only class (and empty class
> >       // with the is_declaration flag set) that carries a non-zero
> >diff --git a/src/abg-default-reporter.cc b/src/abg-default-reporter.cc
> >index 04e2bb76..19dec4cb 100644
> >--- a/src/abg-default-reporter.cc
> >+++ b/src/abg-default-reporter.cc
> >@@ -94,13 +94,33 @@ default_reporter::report(const enum_diff& d, ostream&
> out,
> >                                                  d.second_subject(),
> >                                                  "enum type");
> >
> >-  string name = d.first_enum()->get_pretty_representation();
> >-
> >   enum_type_decl_sptr first = d.first_enum(), second = d.second_enum();
> >
> >-  report_name_size_and_alignment_changes(first, second, d.context(),
> >+  const diff_context_sptr& ctxt = d.context();
> >+
> >+  string name = first->get_pretty_representation();
>
> const
>

Ack.


> >+
> >+  // Report enum decl-only <-> definition changes.
> >+  if (ctxt->get_allowed_category() & ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY)
> >+    if (filtering::has_enum_decl_only_def_change(first, second))
> >+      {
> >+      string was =
> >+        first->get_is_declaration_only()
> >+        ? " was a declaration-only enum type"
> >+        : " was a defined enum type";
> >+
> >+      string is_now =
> >+        second->get_is_declaration_only()
> >+        ? " and is now a declaration-only enum type"
> >+        : " and is now a defined enum type";
> >+
> >+      out << indent << "enum type " << name << was << is_now << "\n";
> >+      return;
> >+      }
> >+
> >+  report_name_size_and_alignment_changes(first, second, ctxt,
> >                                        out, indent);
> >-  maybe_report_diff_for_member(first, second, d.context(), out, indent);
> >+  maybe_report_diff_for_member(first, second, ctxt, out, indent);
> >
> >   //underlying type
> >   d.underlying_type_diff()->report(out, indent);
> >@@ -165,12 +185,12 @@ default_reporter::report(const enum_diff& d,
> ostream& out,
> >             << "' from value '"
> >             << i->first.get_value() << "' to '"
> >             << i->second.get_value() << "'";
> >-        report_loc_info(second, *d.context(), out);
> >+        report_loc_info(second, *ctxt, out);
> >         out << "\n";
> >       }
> >     }
> >
> >-  if (d.context()->show_leaf_changes_only())
> >+  if (ctxt->show_leaf_changes_only())
> >     maybe_report_interfaces_impacted_by_diff(&d, out, indent);
> >
> >   d.reported_once(true);
> >@@ -844,7 +864,7 @@ default_reporter::report(const class_or_union_diff& d,
> >
> >   const diff_context_sptr& ctxt = d.context();
> >
> >-  // Report class decl-only -> definition change.
> >+  // Report class decl-only <-> definition change.
> >   if (ctxt->get_allowed_category() & CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY)
> >     if (filtering::has_class_decl_only_def_change(first, second))
> >       {
> >diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
> >index 850281ad..5748460e 100644
> >--- a/src/abg-dwarf-reader.cc
> >+++ b/src/abg-dwarf-reader.cc
> >@@ -139,6 +139,11 @@ typedef unordered_map<Dwarf_Off, class_decl_sptr>
> die_class_map_type;
> > /// corresponding class_or_union_sptr.
> > typedef unordered_map<Dwarf_Off, class_or_union_sptr>
> die_class_or_union_map_type;
> >
> >+/// Convenience typedef for a map which key is the offset of a dwarf
> >+/// die, (given by dwarf_dieoffset()) and which value is the
> >+/// corresponding enum_type_decl_sptr.
> >+typedef unordered_map<Dwarf_Off, enum_type_decl_sptr> die_enum_map_type;
> >+
> > /// Convenience typedef for a map which key the offset of a dwarf die
> > /// and which value is the corresponding function_decl.
> > typedef unordered_map<Dwarf_Off, function_decl_sptr>
> die_function_decl_map_type;
> >@@ -198,6 +203,10 @@ typedef unordered_map<Dwarf_Off, Dwarf_Off>
> offset_offset_map_type;
> > /// value is a vector of smart pointer to a class.
> > typedef unordered_map<string, classes_type> string_classes_map;
> >
> >+/// Convenience typedef for a map which key is a string and which
> >+/// value is a vector of smart pointer to a enum.
> >+typedef unordered_map<string, enums_type> string_enums_map;
> >+
> > /// The abstraction of the place where a partial unit has been
> > /// imported.  This is what the DW_TAG_imported_unit DIE expresses.
> > ///
> >@@ -2273,6 +2282,9 @@ public:
> >   die_class_or_union_map_type die_wip_classes_map_;
> >   die_class_or_union_map_type alternate_die_wip_classes_map_;
> >   die_class_or_union_map_type type_unit_die_wip_classes_map_;
> >+  die_enum_map_type           die_wip_enums_map_;
> >+  die_enum_map_type           alternate_die_wip_enums_map_;
> >+  die_enum_map_type           type_unit_die_wip_enums_map_;
> >   die_function_type_map_type  die_wip_function_types_map_;
> >   die_function_type_map_type  alternate_die_wip_function_types_map_;
> >   die_function_type_map_type  type_unit_die_wip_function_types_map_;
> >@@ -2282,6 +2294,7 @@ public:
> >   vector<Dwarf_Off>           type_unit_types_to_canonicalize_;
> >   vector<type_base_sptr>      extra_types_to_canonicalize_;
> >   string_classes_map          decl_only_classes_map_;
> >+  string_enums_map            decl_only_enums_map_;
> >   die_tu_map_type             die_tu_map_;
> >   corpus_group_sptr           cur_corpus_group_;
> >   corpus_sptr                 cur_corpus_;
> >@@ -4340,6 +4353,44 @@ public:
> >     return die_wip_classes_map_;
> >   }
> >
> >+  /// Getter of a map that associates a die that represents a
> >+  /// enum with the declaration of the enum, while the enum
> >+  /// is being constructed.
> >+  ///
> >+  /// @param source where the DIE is from.
> >+  ///
> >+  /// @return the map that associates a DIE to the enum that is being
> >+  /// built.
> >+  const die_enum_map_type&
> >+  die_wip_enums_map(die_source source) const
> >+  {return const_cast<read_context*>(this)->die_wip_enums_map(source);}
>
> Give the comment further down, this can also be templated.
>

The WIP stuff *can* actually be simplified by going up the existing
inheritance hierarchy.


> >+
> >+  /// Getter of a map that associates a die that represents a
> >+  /// enum with the declaration of the enum, while the enum
> >+  /// is being constructed.
> >+  ///
> >+  /// @param source where the DIE comes from.
> >+  ///
> >+  /// @return the map that associates a DIE to the enum that is being
> >+  /// built.
>
> This should be a template over the map type to consolidate with
> die_wip_classes_map.
>
> >+  die_enum_map_type&
> >+  die_wip_enums_map(die_source source)
> >+  {
> >+    switch (source)
> >+      {
> >+      case PRIMARY_DEBUG_INFO_DIE_SOURCE:
> >+      break;
> >+      case ALT_DEBUG_INFO_DIE_SOURCE:
> >+      return alternate_die_wip_enums_map_;
> >+      case TYPE_UNIT_DIE_SOURCE:
> >+      return type_unit_die_wip_enums_map_;
> >+      case NO_DEBUG_INFO_DIE_SOURCE:
> >+      case NUMBER_OF_DIE_SOURCES:
> >+      ABG_ASSERT_NOT_REACHED;
> >+      }
> >+    return die_wip_enums_map_;
> >+  }
> >+
> >   /// Getter for a map that associates a die (that represents a
> >   /// function type) whith a function type, while the function type is
> >   /// being constructed (WIP == work in progress).
> >@@ -4620,6 +4671,203 @@ public:
> >       }
> >   }
> >
> >+  /// Getter for the map of declaration-only enums that are to be
> >+  /// resolved to their definition enums by the end of the corpus
> >+  /// loading.
> >+  ///
> >+  /// @return a map of string -> vector of enums where the key is
> >+  /// the fully qualified name of the enum and the value is the
> >+  /// vector of declaration-only enum.
> >+  const string_enums_map&
> >+  declaration_only_enums() const
> >+  {return decl_only_enums_map_;}
> >+
> >+  /// Getter for the map of declaration-only enums that are to be
> >+  /// resolved to their definition enums by the end of the corpus
> >+  /// loading.
> >+  ///
> >+  /// @return a map of string -> vector of enums where the key is
> >+  /// the fully qualified name of the enum and the value is the
> >+  /// vector of declaration-only enum.
> >+  string_enums_map&
> >+  declaration_only_enums()
> >+  {return decl_only_enums_map_;}
> >+
> >+  /// If a given enum is a declaration-only enum then stash it on
> >+  /// the side so that at the end of the corpus reading we can resolve
> >+  /// it to its definition.
> >+  ///
> >+  /// @param enom the enum to consider.
> >+  void
> >+
> maybe_schedule_declaration_only_enum_for_resolution(enum_type_decl_sptr&
> enom)
> >+  {
> >+    if (enom->get_is_declaration_only()
> >+      && enom->get_definition_of_declaration() == 0)
>
>   == NULL
>

It's copy/paste so the right thing to do is adjust this stylistic issue in
another commit (or decide not to adjust it).


> >+      {
> >+      string qn = enom->get_qualified_name();
> >+      string_enums_map::iterator record =
> >+        declaration_only_enums().find(qn);
> >+      if (record == declaration_only_enums().end())
> >+        declaration_only_enums()[qn].push_back(enom);
> >+      else
> >+        record->second.push_back(enom);
> >+      }
>
>    Isn't that just
>     declaration_only_enums()[qn].push_back(enom);    ?
>

Yes. Similar comment to previous.


> >+  }
>
> What comes now is a lot of code duplication. There must be a way to
> reuse the existing code and I once more suggest using templates with
> maybe intermediate functions to get the correct helper functions and
> maps.
>

The code duplication would be eliminable if there were a part of the
inheritance hierarchy that represented "type declarations, potentially
forward-declared". We'd want a common root for struct, union, class and
enum declarations and for the is_declaration_only (et al) machinery to
exist at that level.

This could be a fairly intrusive change.


> I think I would like to see clarified whether we can reduce the
> duplication or what I am missing why we need this to be separate.
>
> I think it would also make it easier to review and judge the patch.
>
> Cheers,
> Matthias
>

I'll simplify what I can, without touching the hierarchy, for further
review.

Regards,
Giuliano.


> >+
> >+  /// Test if a given declaration-only enum has been scheduled for
> >+  /// resolution to a defined enum.
> >+  ///
> >+  /// @param enom the enum to consider for the test.
> >+  ///
> >+  /// @return true iff @p enom is a declaration-only enum and if
> >+  /// it's been scheduled for resolution to a defined enum.
> >+  bool
> >+  is_decl_only_enum_scheduled_for_resolution(enum_type_decl_sptr& enom)
> >+  {
> >+    if (enom->get_is_declaration_only())
> >+      return (declaration_only_enums().find(enom->get_qualified_name())
> >+            != declaration_only_enums().end());
> >+
> >+    return false;
> >+  }
> >+
> >+  /// Walk the declaration-only enums that have been found during
> >+  /// the building of the corpus and resolve them to their definitions.
> >+  void
> >+  resolve_declaration_only_enums()
> >+  {
> >+    vector<string> resolved_enums;
> >+
> >+    for (string_enums_map::iterator i =
> >+         declaration_only_enums().begin();
> >+       i != declaration_only_enums().end();
> >+       ++i)
> >+      {
> >+      bool to_resolve = false;
> >+      for (enums_type::iterator j = i->second.begin();
> >+           j != i->second.end();
> >+           ++j)
> >+        if ((*j)->get_is_declaration_only()
> >+            && ((*j)->get_definition_of_declaration() == 0))
> >+          to_resolve = true;
> >+
> >+      if (!to_resolve)
> >+        {
> >+          resolved_enums.push_back(i->first);
> >+          continue;
> >+        }
> >+
> >+      // Now, for each decl-only enum that have the current name
> >+      // 'i->first', let's try to poke at the fully defined enum
> >+      // that is defined in the same translation unit as the
> >+      // declaration.
> >+      //
> >+      // If we find one enum (defined in the TU of the declaration)
> >+      // that defines the declaration, then the declaration can be
> >+      // resolved to that enum.
> >+      //
> >+      // If no defining enum is found in the TU of the declaration,
> >+      // then there are possibly three cases to consider:
> >+      //
> >+      //   1/ There is exactly one enum that defines the
> >+      //   declaration and that enum is defined in another TU.  In
> >+      //   this case, the declaration is resolved to that
> >+      //   definition.
> >+      //
> >+      //   2/ There are more than one enum that define that
> >+      //   declaration and none of them is defined in the TU of the
> >+      //   declaration.  In this case, the declaration is left
> >+      //   unresolved.
> >+      //
> >+      //   3/ No enum defines the declaration.  In this case, the
> >+      //   declaration is left unresoved.
> >+
> >+      // So get the enums that might define the current
> >+      // declarations which name is i->first.
> >+      const type_base_wptrs_type *enums =
> >+        lookup_enum_types(i->first, *current_corpus());
> >+      if (!enums)
> >+        continue;
> >+
> >+      unordered_map<string, enum_type_decl_sptr> per_tu_enum_map;
> >+      for (type_base_wptrs_type::const_iterator c = enums->begin();
> >+           c != enums->end();
> >+           ++c)
> >+        {
> >+          enum_type_decl_sptr enom = is_enum_type(type_base_sptr(*c));
> >+          ABG_ASSERT(enom);
> >+
> >+          enom = is_enum_type(look_through_decl_only_enum(enom));
> >+          if (enom->get_is_declaration_only())
> >+            continue;
> >+
> >+          string tu_path =
> enom->get_translation_unit()->get_absolute_path();
> >+          if (tu_path.empty())
> >+            continue;
> >+
> >+          // Build a map that associates the translation unit path
> >+          // to the enum (that potentially defines the declarations
> >+          // that we consider) that are defined in that translation unit.
> >+          per_tu_enum_map[tu_path] = enom;
> >+        }
> >+
> >+      if (!per_tu_enum_map.empty())
> >+        {
> >+          // Walk the declarations to resolve and resolve them
> >+          // either to the definitions that are in the same TU as
> >+          // the declaration, or to the definition found elsewhere,
> >+          // if there is only one such definition.
> >+          for (enums_type::iterator j = i->second.begin();
> >+               j != i->second.end();
> >+               ++j)
> >+            {
> >+              if ((*j)->get_is_declaration_only()
> >+                  && ((*j)->get_definition_of_declaration() == 0))
> >+                {
> >+                  string tu_path =
> >+                    (*j)->get_translation_unit()->get_absolute_path();
> >+                  unordered_map<string,
> enum_type_decl_sptr>::const_iterator e =
> >+                    per_tu_enum_map.find(tu_path);
> >+                  if (e != per_tu_enum_map.end())
> >+                    (*j)->set_definition_of_declaration(e->second);
> >+                  else if (per_tu_enum_map.size() == 1)
> >+                    (*j)->set_definition_of_declaration
> >+                      (per_tu_enum_map.begin()->second);
> >+                }
> >+            }
> >+          resolved_enums.push_back(i->first);
> >+        }
> >+      }
> >+
> >+    size_t num_decl_only_enums = declaration_only_enums().size(),
> >+      num_resolved = resolved_enums.size();
> >+    if (show_stats())
> >+      cerr << "resolved " << num_resolved
> >+         << " enum declarations out of "
> >+         << num_decl_only_enums
> >+         << "\n";
> >+
> >+    for (vector<string>::const_iterator i = resolved_enums.begin();
> >+       i != resolved_enums.end();
> >+       ++i)
> >+      declaration_only_enums().erase(*i);
> >+
> >+    for (string_enums_map::iterator i = declaration_only_enums().begin();
> >+       i != declaration_only_enums().end();
> >+       ++i)
> >+      {
> >+      if (show_stats())
> >+        {
> >+          if (i == declaration_only_enums().begin())
> >+            cerr << "Here are the "
> >+                 << num_decl_only_enums - num_resolved
> >+                 << " unresolved enum declarations:\n";
> >+          else
> >+            cerr << "    " << i->first << "\n";
> >+        }
> >+      }
> >+  }
> >+
> >   /// Some functions described by DWARF may have their linkage name
> >   /// set, but no link to their actual underlying elf symbol.  When
> >   /// these are virtual member functions, comparing the enclosing type
> >@@ -13386,18 +13634,32 @@ build_enum_type(read_context&        ctxt,
> >   if (tag != DW_TAG_enumeration_type)
> >     return result;
> >
> >+  die_source source;
> >+  ABG_ASSERT(ctxt.get_die_source(die, source));
> >+  {
> >+    die_enum_map_type::const_iterator i =
> >+      ctxt.die_wip_enums_map(source).find(dwarf_dieoffset(die));
> >+    if (i != ctxt.die_wip_enums_map(source).end())
> >+      {
> >+      enum_type_decl_sptr u = is_enum_type(i->second);
> >+      ABG_ASSERT(u);
> >+      return u;
> >+      }
> >+  }
> >+
> >   string name, linkage_name;
> >   location loc;
> >   die_loc_and_name(ctxt, die, loc, name, linkage_name);
> >+  bool is_declaration_only = die_is_declaration_only(die);
> >
> >-  bool enum_is_anonymous = false;
> >+  bool is_anonymous = false;
> >   // If the enum is anonymous, let's give it a name.
> >   if (name.empty())
> >     {
> >       name = get_internal_anonymous_die_prefix_name(die);
> >       ABG_ASSERT(!name.empty());
> >       // But we remember that the type is anonymous.
> >-      enum_is_anonymous = true;
> >+      is_anonymous = true;
> >
> >       if (size_t s = scope->get_num_anonymous_member_enums())
> >       name = build_internal_anonymous_die_name(name, s);
> >@@ -13409,7 +13671,7 @@ build_enum_type(read_context&  ctxt,
> >   // representation (name) and location can be later detected as being
> >   // for the same type.
> >
> >-  if (!enum_is_anonymous)
> >+  if (!is_anonymous)
> >     {
> >       if (use_odr)
> >       {
> >@@ -13442,6 +13704,7 @@ build_enum_type(read_context&  ctxt,
> >   uint64_t size = 0;
> >   if (die_unsigned_constant_attribute(die, DW_AT_byte_size, size))
> >     size *= 8;
> >+  bool is_artificial = die_is_artificial(die);
> >
> >   // for now we consider that underlying types of enums are all anonymous
> >   bool enum_underlying_type_is_anonymous= true;
> >@@ -13474,8 +13737,6 @@ build_enum_type(read_context&  ctxt,
> >       while (dwarf_siblingof(&child, &child) == 0);
> >     }
> >
> >-  bool is_artificial = die_is_artificial(die);
> >-
> >   // DWARF up to version 4 (at least) doesn't seem to carry the
> >   // underlying type, so let's create an artificial one here, which
> >   // sole purpose is to be passed to the constructor of the
> >@@ -13491,9 +13752,13 @@ build_enum_type(read_context& ctxt,
> >   t = dynamic_pointer_cast<type_decl>(d);
> >   ABG_ASSERT(t);
> >   result.reset(new enum_type_decl(name, loc, t, enms, linkage_name));
> >-  result->set_is_anonymous(enum_is_anonymous);
> >+  result->set_is_anonymous(is_anonymous);
> >+  result->set_is_declaration_only(is_declaration_only);
> >   result->set_is_artificial(is_artificial);
> >   ctxt.associate_die_to_type(die, result, where_offset);
> >+
> >+  ctxt.maybe_schedule_declaration_only_enum_for_resolution(result);
> >+
> >   return result;
> > }
> >
> >@@ -16226,6 +16491,24 @@ read_debug_info_into_corpus(read_context& ctxt)
> >       }
> >   }
> >
> >+  {
> >+    tools_utils::timer t;
> >+    if (ctxt.do_log())
> >+      {
> >+      cerr << "resolving declaration only enums ...";
> >+      t.start();
> >+      }
> >+    ctxt.resolve_declaration_only_enums();
> >+    if (ctxt.do_log())
> >+      {
> >+      t.stop();
> >+      cerr << " DONE@" << ctxt.current_corpus()->get_path()
> >+           << ":"
> >+           << t
> >+           <<"\n";
> >+      }
> >+  }
> >+
> >   {
> >     tools_utils::timer t;
> >     if (ctxt.do_log())
> >@@ -16660,7 +16943,18 @@ build_ir_node_from_die(read_context&  ctxt,
> >
> >     case DW_TAG_enumeration_type:
> >       {
> >-      if (!type_is_suppressed(ctxt, scope, die))
> >+      bool type_is_private = false;
> >+      bool type_suppressed =
> >+        type_is_suppressed(ctxt, scope, die, type_is_private);
> >+      if (type_suppressed && type_is_private)
> >+        // The type is suppressed because it's private.  If other
> >+        // non-suppressed and declaration-only instances of this
> >+        // type exist in the current corpus, then it means those
> >+        // non-suppressed instances are opaque versions of the
> >+        // suppressed private type.  Lets return one of these opaque
> >+        // types then.
> >+        result = get_opaque_version_of_type(ctxt, scope, die,
> where_offset);
> >+      else if (!type_suppressed)
> >         {
> >           enum_type_decl_sptr e = build_enum_type(ctxt, die, scope,
> >                                                   where_offset);
> >diff --git a/src/abg-ir.cc b/src/abg-ir.cc
> >index 27831352..bfdc5679 100644
> >--- a/src/abg-ir.cc
> >+++ b/src/abg-ir.cc
> >@@ -7674,6 +7674,38 @@ typedef_decl*
> > is_typedef(type_base* t)
> > {return dynamic_cast<typedef_decl*>(t);}
> >
> >+/// Test if a type is a enum. This function looks through typedefs.
> >+///
> >+/// @parm t the type to consider.
> >+///
> >+/// @return the enum_decl if @p t is a enum_decl or null otherwise.
> >+enum_type_decl_sptr
> >+is_compatible_with_enum_type(const type_base_sptr& t)
> >+{
> >+  if (!t)
> >+    return enum_type_decl_sptr();
> >+
> >+  // Normally we should strip typedefs entirely, but this is
> >+  // potentially costly, especially on binaries with huge changesets
> >+  // like the Linux Kernel.  So we just get the leaf types for now.
> >+  //
> >+  // Maybe there should be an option by which users accepts to pay the
> >+  // CPU usage toll in exchange for finer filtering?
> >+
> >+  // type_base_sptr ty = strip_typedef(t);
> >+  type_base_sptr ty = peel_typedef_type(t);;
> >+  return is_enum_type(ty);
> >+}
> >+
> >+/// Test if a type is a enum. This function looks through typedefs.
> >+///
> >+/// @parm t the type to consider.
> >+///
> >+/// @return the enum_decl if @p t is a enum_decl or null otherwise.
> >+enum_type_decl_sptr
> >+is_compatible_with_enum_type(const decl_base_sptr& t)
> >+{return is_compatible_with_enum_type(is_type(t));}
> >+
> > /// Test if a decl is an enum_type_decl
> > ///
> > /// @param d the decl to test for.
> >@@ -8057,6 +8089,50 @@ look_through_decl_only_class(class_or_union_sptr
> klass)
> >   return result;
> > }
> >
> >+/// If a enum is a decl-only enum, get its definition.
> >+/// Otherwise, just return the initial enum.
> >+///
> >+/// @param the_enum the enum to consider.
> >+///
> >+/// @return either the definition of the enum, or the enum itself.
> >+enum_type_decl_sptr
> >+look_through_decl_only_enum(const enum_type_decl& the_enum)
> >+{
> >+  enum_type_decl_sptr enom;
> >+  if (the_enum.get_is_declaration_only())
> >+    enom = the_enum.get_definition_of_declaration();
> >+
> >+  if (!enom)
> >+    return enom;
> >+
> >+  while (enom
> >+       && enom->get_is_declaration_only()
> >+       && enom->get_definition_of_declaration())
> >+    enom = enom->get_definition_of_declaration();
> >+
> >+  ABG_ASSERT(enom);
> >+  return enom;
> >+}
> >+
> >+/// If a enum is a decl-only enum, get its definition.
> >+/// Otherwise, just return the initial enum.
> >+///
> >+/// @param enom the enum to consider.
> >+///
> >+/// @return either the definition of the enum, or the enum itself.
> >+enum_type_decl_sptr
> >+look_through_decl_only_enum(enum_type_decl_sptr enom)
> >+{
> >+  if (!enom)
> >+    return enom;
> >+
> >+  enum_type_decl_sptr result = look_through_decl_only_enum(*enom);
> >+  if (!result)
> >+    result = enom;
> >+
> >+  return result;
> >+}
> >+
> > /// Tests if a declaration is a variable declaration.
> > ///
> > /// @param decl the decl to test.
> >@@ -9915,6 +9991,37 @@ lookup_enum_type(const interned_string&
> qualified_name, const corpus& corp)
> >   return result;
> > }
> >
> >+/// Look into a given corpus to find the enum type*s* that have a
> >+/// given qualified name.
> >+///
> >+/// @param qualified_name the qualified name of the type to look for.
> >+///
> >+/// @param corp the corpus to look into.
> >+///
> >+/// @return the vector of enum types that which name is @p
> qualified_name.
> >+const type_base_wptrs_type *
> >+lookup_enum_types(const interned_string& qualified_name, const corpus&
> corp)
> >+{
> >+  const istring_type_base_wptrs_map_type& m =
> corp.get_types().enum_types();
> >+
> >+  return lookup_types_in_map(qualified_name, m);
> >+}
> >+
> >+/// Look into a given corpus to find the enum type*s* that have a
> >+/// given qualified name.
> >+///
> >+/// @param qualified_name the qualified name of the type to look for.
> >+///
> >+/// @param corp the corpus to look into.
> >+///
> >+/// @return the vector of enum types that which name is @p
> qualified_name.
> >+const type_base_wptrs_type*
> >+lookup_enum_types(const string& qualified_name, const corpus& corp)
> >+{
> >+  interned_string s = corp.get_environment()->intern(qualified_name);
> >+  return lookup_enum_types(s, corp);
> >+}
> >+
> > /// Look up an @ref enum_type_decl from a given corpus, by its location.
> > ///
> > /// @param loc the location to consider.
> >@@ -14724,17 +14831,26 @@ class enum_type_decl::priv
> > {
> >   type_base_sptr      underlying_type_;
> >   enumerators         enumerators_;
> >+  decl_base_sptr              declaration_;
> >+  enum_type_decl_wptr definition_of_declaration_;
> >+  enum_type_decl*     naked_definition_of_declaration_;
> >+  bool                        is_declaration_only_;
> >
> >   friend class enum_type_decl;
> >
> >-  priv();
> >-
> > public:
> >
> >+  priv()
> >+    : naked_definition_of_declaration_(),
> >+      is_declaration_only_(true)
> >+  {}
> >+
> >   priv(type_base_sptr underlying_type,
> >        enumerators& enumerators)
> >     : underlying_type_(underlying_type),
> >-      enumerators_(enumerators)
> >+      enumerators_(enumerators),
> >+      naked_definition_of_declaration_(),
> >+      is_declaration_only_(false)
> >   {}
> > }; // end class enum_type_decl::priv
> >
> >@@ -14775,6 +14891,25 @@ enum_type_decl::enum_type_decl(const string&
> name,
> >     e->set_enum_type(this);
> > }
> >
> >+/// Constructor for instances of enum_type_decl that represent a
> >+/// declaration without definition.
> >+///
> >+/// @param env the environment we are operating from.
> >+///
> >+/// @param name the name of the enum.
> >+enum_type_decl::enum_type_decl(const environment*     env,
> >+                             const string&            name)
> >+  : type_or_decl_base(env,
> >+                    ENUM_TYPE
> >+                    | ABSTRACT_TYPE_BASE
> >+                    | ABSTRACT_DECL_BASE),
> >+    type_base(env, 0, 0),
> >+    decl_base(env, name, location(), name),
> >+    priv_(new priv)
> >+{
> >+  runtime_type_instance(this);
> >+}
> >+
> > /// Return the underlying type of the enum.
> > type_base_sptr
> > enum_type_decl::get_underlying_type() const
> >@@ -14790,6 +14925,100 @@ enum_type_decl::enumerators&
> > enum_type_decl::get_enumerators()
> > {return priv_->enumerators_;}
> >
> >+/// Set the definition of this declaration-only @ref enum_type_decl.
> >+///
> >+/// @param d the new definition to set.
> >+void
> >+enum_type_decl::set_definition_of_declaration(enum_type_decl_sptr d)
> >+{
> >+  ABG_ASSERT(get_is_declaration_only());
> >+  priv_->definition_of_declaration_ = d;
> >+  if (d->get_canonical_type())
> >+    type_base::priv_->canonical_type = d->get_canonical_type();
> >+
> >+  priv_->naked_definition_of_declaration_ = d.get();
> >+}
> >+
> >+/// If this @ref enum_type_decl_sptr is declaration-only, get its
> >+/// definition, if any.
> >+///
> >+/// @return the definition of this decl-only enum.
> >+const enum_type_decl_sptr
> >+enum_type_decl::get_definition_of_declaration() const
> >+{
> >+  if (priv_->definition_of_declaration_.expired())
> >+    return enum_type_decl_sptr();
> >+  return enum_type_decl_sptr(priv_->definition_of_declaration_);
> >+}
> >+
> >+///  If this @ref enum_type_decl is declaration-only, get its
> >+///  definition, if any.
> >+///
> >+/// Note that this function doesn't return a smart pointer, but rather
> >+/// the underlying pointer managed by the smart pointer.  So it's as
> >+/// fast as possible.  This getter is to be used in code paths that are
> >+/// proven to be performance hot spots; especially, when comparing
> >+/// sensitive types like enums.  Those are compared extremely frequently
> >+/// and thus, their access to the definition of declaration must be
> >+/// fast.
> >+///
> >+/// @return the definition of the enum.
> >+const enum_type_decl*
> >+enum_type_decl::get_naked_definition_of_declaration() const
> >+{return priv_->naked_definition_of_declaration_;}
> >+
> >+/// If this @ref enum_type_decl_sptr is a definition, get its earlier
> >+/// declaration.
> >+///
> >+/// @return the earlier declaration of the enum, if any.
> >+decl_base_sptr
> >+enum_type_decl::get_earlier_declaration() const
> >+{return priv_->declaration_;}
> >+
> >+/// set the earlier declaration of this @ref enum_type_decl definition.
> >+///
> >+/// @param declaration the earlier declaration to set.  Note that it's
> >+/// set only if it's a pure declaration.
> >+void
> >+enum_type_decl::set_earlier_declaration(decl_base_sptr declaration)
> >+{
> >+  enum_type_decl_sptr cl =
> dynamic_pointer_cast<enum_type_decl>(declaration);
> >+  if (cl && cl->get_is_declaration_only())
> >+    priv_->declaration_ = declaration;
> >+}
> >+
> >+/// Test if a @ref enum_type_decl is a declaration-only @ref
> >+/// enum_type_decl.
> >+///
> >+/// @return true iff the current @ref enum_type_decl is a
> >+/// declaration-only @ref enum_type_decl.
> >+bool
> >+enum_type_decl::get_is_declaration_only() const
> >+{return priv_->is_declaration_only_;}
> >+
> >+/// Set a flag saying if the @ref enum_type_decl is a declaration-only
> >+/// @ref enum_type_decl.
> >+///
> >+/// @param f true if the @ref enum_type_decl is a decalaration-only
> >+/// @ref enum_type_decl.
> >+void
> >+enum_type_decl::set_is_declaration_only(bool f)
> >+{
> >+  bool update_types_lookup_map = !f && priv_->is_declaration_only_;
> >+
> >+  priv_->is_declaration_only_ = f;
> >+
> >+  if (update_types_lookup_map)
> >+    if (scope_decl* s = get_scope())
> >+      {
> >+      scope_decl::declarations::iterator i;
> >+      if (s->find_iterator_for_member(this, i))
> >+        maybe_update_types_lookup_map(*i);
> >+      else
> >+        ABG_ASSERT_NOT_REACHED;
> >+      }
> >+}
> >+
> > /// Get the pretty representation of the current instance of @ref
> > /// enum_type_decl.
> > ///
> >--
> >2.26.1.301.g55bc3eb7cb9-goog
> >
>

Hello,

I think this is a very interesting and needed task.  It's a bit
involved, given that it touches many parts of the codebase so I really
appreciate you diving into this.  Really, thank you for doing this.  And
I really find that your patch set is a great start.  Honest.

To help with conveying how I think we should shape this, I have put your
patches as well as some changes that I talk about in this review in the
dodji/incomp-enums branch at
https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/dodji/incomp-enums.

You can start from that branch to further amend the patch series
(following the review round) or not, at your convenience.  In any case,
I'll sometimes refer to a patch in that branch when I try to convey an
idea about a particular topic in your patch set.

I think the three main things that are missing from this patch set are

  1/ the support for serializing declaration-only enums.  I guess this
  would be like what is done for declaration-only classes in
  write_class_decl_opening_tag by invoking
  write_class_or_union_is_declaration_only.

  2/ support generating an opaque type from an enum when that enum has
  been deemed private by a type supression specification.  This should
  be done by get_opaque_version_of_type in abg-dwarf-reader.cc.  I have
  put up a patch for this in dodji/incomp-enums at
  https://sourceware.org/git/?p=libabigail.git;a=commit;h=4e361d914e8e6c34d5e00f570551b4b5a545fd82

  3/ write tests that exercise
     * opaque enum type building from suppression specifications
     * resolution of declaration-only enums to their right fully defined counterparts.
     * serialization of declaration-only enums


There might be other things to add/change (like ensuring that after
decl-only enums resolution is performed, an unresolved decl-only enum
doesn't equal a fully defined enum)  but I think we can get to those
later once these fundamental pieces are dealt with first.

Giuliano Procida <gprocida@google.com> a écrit:

[...]

> diff --git a/include/abg-fwd.h b/include/abg-fwd.h
> index 1aab70a6..d1cf0322 100644
> --- a/include/abg-fwd.h
> +++ b/include/abg-fwd.h
> @@ -154,9 +154,15 @@ typedef weak_ptr<typedef_decl> typedef_decl_wptr;
>  
>  class enum_type_decl;
>  
> -/// Convenience typedef for shared pointer on enum_type_decl.
> +/// Convenience typedef for shared pointer to a @ref enum_type_decl.
>  typedef shared_ptr<enum_type_decl> enum_type_decl_sptr;

Your fix is correct, but I think it should probably go into a separate
commit.

[...]

> --- a/include/abg-ir.h
> +++ b/include/abg-ir.h
> @@ -2465,6 +2465,8 @@ public:
>  		 const string&		mangled_name = "",
>  		 visibility		vis = VISIBILITY_DEFAULT);
>  
> +  enum_type_decl(const environment* env, const string& name);

Declaring a new constructor for this type here is not necessary.  So we
should drop this change.  Please see later down below where I comment
about the changes in build_enum_type.

[...]


>  /// Test if a class_or_union_diff carries a change in which the two
>  /// classes are different by the fact that one is a decl-only and the
>  /// other one is defined.
>  ///
>  /// @param diff the diff node to consider.
> -////
> -//// @return true if the class_or_union_diff carries a change in which
> +///
> +/// @return true if the class_or_union_diff carries a change in which
>  /// the two classes are different by the fact that one is a decl-only
>  /// and the other one is defined.
>  bool

This change should go in a different "misc style fixes" commit.

[...]

> +++ b/src/abg-comparison.cc

[...]


> -    if (c & FN_RETURN_TYPE_CV_CHANGE_CATEGORY)
> +  if (c & FN_RETURN_TYPE_CV_CHANGE_CATEGORY)

Likewise.

[...]

> -   if (c & VAR_TYPE_CV_CHANGE_CATEGORY)
> +  if (c & VAR_TYPE_CV_CHANGE_CATEGORY)

Likewise.

[...]

> -    if (c & VOID_PTR_TO_PTR_CHANGE_CATEGORY)
> +  if (c & VOID_PTR_TO_PTR_CHANGE_CATEGORY)

Likewise.

[...]

> -    if (c & BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY)
> +  if (c & BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY)

Likewise.

[...]

> --- a/src/abg-default-reporter.cc
> +++ b/src/abg-default-reporter.cc
> @@ -94,13 +94,33 @@ default_reporter::report(const enum_diff& d, ostream& out,
>  						   d.second_subject(),
>  						   "enum type");
>  
> -  string name = d.first_enum()->get_pretty_representation();
> -

This change is ...


>    enum_type_decl_sptr first = d.first_enum(), second = d.second_enum();
>  
> -  report_name_size_and_alignment_changes(first, second, d.context(),
> +  const diff_context_sptr& ctxt = d.context();
> +
> +  string name = first->get_pretty_representation();

... unnecessary.

The s/d.context()/ctxt/ changes in this function are unnecessary as
well.

[...]

>    d.reported_once(true);
> @@ -844,7 +864,7 @@ default_reporter::report(const class_or_union_diff& d,
>  
>    const diff_context_sptr& ctxt = d.context();
>  
> -  // Report class decl-only -> definition change.
> +  // Report class decl-only <-> definition change.

This change is unrelated to the current patch.

[...]

> +++ b/src/abg-dwarf-reader.cc

[...]

> @@ -139,6 +139,11 @@ typedef unordered_map<Dwarf_Off, class_decl_sptr> die_class_map_type;
>  /// corresponding class_or_union_sptr.
>  typedef unordered_map<Dwarf_Off, class_or_union_sptr> die_class_or_union_map_type;
>  
> +/// Convenience typedef for a map which key is the offset of a dwarf
> +/// die, (given by dwarf_dieoffset()) and which value is the
> +/// corresponding enum_type_decl_sptr.
> +typedef unordered_map<Dwarf_Off, enum_type_decl_sptr> die_enum_map_type;
> +

Adding this typedef is unnecessary because ...

[...]

> @@ -2273,6 +2282,9 @@ public:
>    die_class_or_union_map_type	die_wip_classes_map_;
>    die_class_or_union_map_type	alternate_die_wip_classes_map_;
>    die_class_or_union_map_type	type_unit_die_wip_classes_map_;
> +  die_enum_map_type		die_wip_enums_map_;
> +  die_enum_map_type		alternate_die_wip_enums_map_;
> +  die_enum_map_type		type_unit_die_wip_enums_map_;

... these new data members are unnecessary.  Because enum types are
built atomically, they don't have to go through a 'work-in-progress'
process.  That process is only suited for classes and function types
because these are built gradually and thus stay non-fully constructed
for a certain time during which other types are built.  Hence these
transiently non fully built types (a.k.a WIP types) are 'marked' as
such.

[...]


> +  /// Getter of a map that associates a die that represents a
> +  /// enum with the declaration of the enum, while the enum
> +  /// is being constructed.
> +  ///
> +  /// @param source where the DIE is from.
> +  ///
> +  /// @return the map that associates a DIE to the enum that is being
> +  /// built.
> +  const die_enum_map_type&
> +  die_wip_enums_map(die_source source) const
> +  {return const_cast<read_context*>(this)->die_wip_enums_map(source);}

This accessor is unnecessary as well.

[...]

> +
> +  /// Getter of a map that associates a die that represents a
> +  /// enum with the declaration of the enum, while the enum
> +  /// is being constructed.
> +  ///
> +  /// @param source where the DIE comes from.
> +  ///
> +  /// @return the map that associates a DIE to the enum that is being
> +  /// built.
> +  die_enum_map_type&
> +  die_wip_enums_map(die_source source)

Likewise.

[...]

> @@ -16660,7 +16943,18 @@ build_ir_node_from_die(read_context&	ctxt,
>  
>      case DW_TAG_enumeration_type:
>        {
> -	if (!type_is_suppressed(ctxt, scope, die))
> +	bool type_is_private = false;
> +	bool type_suppressed =
> +	  type_is_suppressed(ctxt, scope, die, type_is_private);
> +	if (type_suppressed && type_is_private)
> +	  // The type is suppressed because it's private.  If other
> +	  // non-suppressed and declaration-only instances of this
> +	  // type exist in the current corpus, then it means those
> +	  // non-suppressed instances are opaque versions of the
> +	  // suppressed private type.  Lets return one of these opaque
> +	  // types then.
> +	  result = get_opaque_version_of_type(ctxt, scope, die, where_offset);

Right.  Though, for this to work, get_opaque_version_of_type must be
adapted to function with enum types.  Note that at the moment, it only
functions for classes (not even for unions).  I have added a patch to
dodji/incomp-enums at
https://sourceware.org/git/?p=libabigail.git;a=commit;h=4e361d914e8e6c34d5e00f570551b4b5a545fd82
to do that.  That patch factorizes a some parts of build_enum_type
because it needed it to reuse some them.

> +	else if (!type_suppressed)
>  	  {

[...]

> @@ -13386,18 +13634,32 @@ build_enum_type(read_context&	ctxt,

>    if (tag != DW_TAG_enumeration_type)
>      return result;
>  
> +  die_source source;
> +  ABG_ASSERT(ctxt.get_die_source(die, source));
> +  {
> +    die_enum_map_type::const_iterator i =
> +      ctxt.die_wip_enums_map(source).find(dwarf_dieoffset(die));
> +    if (i != ctxt.die_wip_enums_map(source).end())
> +      {
> +	enum_type_decl_sptr u = is_enum_type(i->second);
> +	ABG_ASSERT(u);
> +	return u;
> +      }
> +  }

As I said earlier, this WIP handling is unnecessary.

[...]

> -  bool enum_is_anonymous = false;
> +  bool is_anonymous = false;

This change in unnecessary.

>    // If the enum is anonymous, let's give it a name.
>    if (name.empty())
>      {
>        name = get_internal_anonymous_die_prefix_name(die);
>        ABG_ASSERT(!name.empty());
>        // But we remember that the type is anonymous.
> -      enum_is_anonymous = true;
> +      is_anonymous = true;

Likewise.

>  
>        if (size_t s = scope->get_num_anonymous_member_enums())
>  	name = build_internal_anonymous_die_name(name, s);
> @@ -13409,7 +13671,7 @@ build_enum_type(read_context&	ctxt,
>    // representation (name) and location can be later detected as being
>    // for the same type.
>  
> -  if (!enum_is_anonymous)
> +  if (!is_anonymous)

Likewise.

>      {
>        if (use_odr)
>  	{
> @@ -13442,6 +13704,7 @@ build_enum_type(read_context&	ctxt,
>    uint64_t size = 0;
>    if (die_unsigned_constant_attribute(die, DW_AT_byte_size, size))
>      size *= 8;
> +  bool is_artificial = die_is_artificial(die);

This change is ...

[...]

>    // for now we consider that underlying types of enums are all anonymous
>    bool enum_underlying_type_is_anonymous= true;
> @@ -13474,8 +13737,6 @@ build_enum_type(read_context&	ctxt,
>        while (dwarf_siblingof(&child, &child) == 0);
>      }
>  
> -  bool is_artificial = die_is_artificial(die);
> -

... unnecessary.

>    // DWARF up to version 4 (at least) doesn't seem to carry the
>    // underlying type, so let's create an artificial one here, which
>    // sole purpose is to be passed to the constructor of the
> @@ -13491,9 +13752,13 @@ build_enum_type(read_context&	ctxt,
>    t = dynamic_pointer_cast<type_decl>(d);
>    ABG_ASSERT(t);
>    result.reset(new enum_type_decl(name, loc, t, enms, linkage_name));
> -  result->set_is_anonymous(enum_is_anonymous);
> +  result->set_is_anonymous(is_anonymous);

Likewise.

[...]


> +++ b/src/abg-ir.cc

[...]


> +/// Constructor for instances of enum_type_decl that represent a
> +/// declaration without definition.
> +///
> +/// @param env the environment we are operating from.
> +///
> +/// @param name the name of the enum.
> +enum_type_decl::enum_type_decl(const environment*	env,
> +			       const string&		name)
> +  : type_or_decl_base(env,
> +		      ENUM_TYPE
> +		      | ABSTRACT_TYPE_BASE
> +		      | ABSTRACT_DECL_BASE),
> +    type_base(env, 0, 0),
> +    decl_base(env, name, location(), name),
> +    priv_(new priv)
> +{
> +  runtime_type_instance(this);
> +}
> +

This constructor is unnecessary.

[...]


> +/// If this @ref enum_type_decl_sptr is a definition, get its earlier
> +/// declaration.
> +///
> +/// @return the earlier declaration of the enum, if any.
> +decl_base_sptr
> +enum_type_decl::get_earlier_declaration() const
> +{return priv_->declaration_;}

I think this is unnecessary.  Classes do have a similar accessor but
it's a relique from ancient times when resolution of decl-only classes
was done differently.  I keep it around to be able to support ancient
abixml files that might be out there somewhere.

> +
> +/// set the earlier declaration of this @ref enum_type_decl definition.
> +///
> +/// @param declaration the earlier declaration to set.  Note that it's
> +/// set only if it's a pure declaration.
> +void
> +enum_type_decl::set_earlier_declaration(decl_base_sptr declaration)
> +{
> +  enum_type_decl_sptr cl = dynamic_pointer_cast<enum_type_decl>(declaration);
> +  if (cl && cl->get_is_declaration_only())
> +    priv_->declaration_ = declaration;
> +}

Likewise.

[...]


Thanks again for working on this.

Cheers,

Hi there.

Thanks for the review comments

I've done a little more work on this and added a couple of your
commits essentially as-is to my series.

Also been playing with github mirroring. I can just as easily repost
to the list, though.

https://github.com/myxoid/libabigail/commits/incomplete-enums
https://github.com/myxoid/libabigail/pull/2 - to allow review
commenting in situ.

Regards,
Giuliano.

On Fri, 22 May 2020 at 11:03, Dodji Seketeli <dodji@seketeli.org> wrote:
>
> Hello,
>
> I think this is a very interesting and needed task.  It's a bit
> involved, given that it touches many parts of the codebase so I really
> appreciate you diving into this.  Really, thank you for doing this.  And
> I really find that your patch set is a great start.  Honest.
>
> To help with conveying how I think we should shape this, I have put your
> patches as well as some changes that I talk about in this review in the
> dodji/incomp-enums branch at
> https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/dodji/incomp-enums.
>
> You can start from that branch to further amend the patch series
> (following the review round) or not, at your convenience.  In any case,
> I'll sometimes refer to a patch in that branch when I try to convey an
> idea about a particular topic in your patch set.
>
> I think the three main things that are missing from this patch set are
>
>   1/ the support for serializing declaration-only enums.  I guess this
>   would be like what is done for declaration-only classes in
>   write_class_decl_opening_tag by invoking
>   write_class_or_union_is_declaration_only.
>
>   2/ support generating an opaque type from an enum when that enum has
>   been deemed private by a type supression specification.  This should
>   be done by get_opaque_version_of_type in abg-dwarf-reader.cc.  I have
>   put up a patch for this in dodji/incomp-enums at
>   https://sourceware.org/git/?p=libabigail.git;a=commit;h=4e361d914e8e6c34d5e00f570551b4b5a545fd82
>
>   3/ write tests that exercise
>      * opaque enum type building from suppression specifications
>      * resolution of declaration-only enums to their right fully defined counterparts.
>      * serialization of declaration-only enums
>
>
> There might be other things to add/change (like ensuring that after
> decl-only enums resolution is performed, an unresolved decl-only enum
> doesn't equal a fully defined enum)  but I think we can get to those
> later once these fundamental pieces are dealt with first.
>
> Giuliano Procida <gprocida@google.com> a écrit:
>
> [...]
>
> > diff --git a/include/abg-fwd.h b/include/abg-fwd.h
> > index 1aab70a6..d1cf0322 100644
> > --- a/include/abg-fwd.h
> > +++ b/include/abg-fwd.h
> > @@ -154,9 +154,15 @@ typedef weak_ptr<typedef_decl> typedef_decl_wptr;
> >
> >  class enum_type_decl;
> >
> > -/// Convenience typedef for shared pointer on enum_type_decl.
> > +/// Convenience typedef for shared pointer to a @ref enum_type_decl.
> >  typedef shared_ptr<enum_type_decl> enum_type_decl_sptr;
>
> Your fix is correct, but I think it should probably go into a separate
> commit.
>
> [...]
>
> > --- a/include/abg-ir.h
> > +++ b/include/abg-ir.h
> > @@ -2465,6 +2465,8 @@ public:
> >                const string&          mangled_name = "",
> >                visibility             vis = VISIBILITY_DEFAULT);
> >
> > +  enum_type_decl(const environment* env, const string& name);
>
> Declaring a new constructor for this type here is not necessary.  So we
> should drop this change.  Please see later down below where I comment
> about the changes in build_enum_type.
>
> [...]
>
>
> >  /// Test if a class_or_union_diff carries a change in which the two
> >  /// classes are different by the fact that one is a decl-only and the
> >  /// other one is defined.
> >  ///
> >  /// @param diff the diff node to consider.
> > -////
> > -//// @return true if the class_or_union_diff carries a change in which
> > +///
> > +/// @return true if the class_or_union_diff carries a change in which
> >  /// the two classes are different by the fact that one is a decl-only
> >  /// and the other one is defined.
> >  bool
>
> This change should go in a different "misc style fixes" commit.
>
> [...]
>
> > +++ b/src/abg-comparison.cc
>
> [...]
>
>
> > -    if (c & FN_RETURN_TYPE_CV_CHANGE_CATEGORY)
> > +  if (c & FN_RETURN_TYPE_CV_CHANGE_CATEGORY)
>
> Likewise.
>
> [...]
>
> > -   if (c & VAR_TYPE_CV_CHANGE_CATEGORY)
> > +  if (c & VAR_TYPE_CV_CHANGE_CATEGORY)
>
> Likewise.
>
> [...]
>
> > -    if (c & VOID_PTR_TO_PTR_CHANGE_CATEGORY)
> > +  if (c & VOID_PTR_TO_PTR_CHANGE_CATEGORY)
>
> Likewise.
>
> [...]
>
> > -    if (c & BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY)
> > +  if (c & BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY)
>
> Likewise.
>
> [...]
>
> > --- a/src/abg-default-reporter.cc
> > +++ b/src/abg-default-reporter.cc
> > @@ -94,13 +94,33 @@ default_reporter::report(const enum_diff& d, ostream& out,
> >                                                  d.second_subject(),
> >                                                  "enum type");
> >
> > -  string name = d.first_enum()->get_pretty_representation();
> > -
>
> This change is ...
>
>
> >    enum_type_decl_sptr first = d.first_enum(), second = d.second_enum();
> >
> > -  report_name_size_and_alignment_changes(first, second, d.context(),
> > +  const diff_context_sptr& ctxt = d.context();
> > +
> > +  string name = first->get_pretty_representation();
>
> ... unnecessary.
>
> The s/d.context()/ctxt/ changes in this function are unnecessary as
> well.
>
> [...]
>
> >    d.reported_once(true);
> > @@ -844,7 +864,7 @@ default_reporter::report(const class_or_union_diff& d,
> >
> >    const diff_context_sptr& ctxt = d.context();
> >
> > -  // Report class decl-only -> definition change.
> > +  // Report class decl-only <-> definition change.
>
> This change is unrelated to the current patch.
>
> [...]
>
> > +++ b/src/abg-dwarf-reader.cc
>
> [...]
>
> > @@ -139,6 +139,11 @@ typedef unordered_map<Dwarf_Off, class_decl_sptr> die_class_map_type;
> >  /// corresponding class_or_union_sptr.
> >  typedef unordered_map<Dwarf_Off, class_or_union_sptr> die_class_or_union_map_type;
> >
> > +/// Convenience typedef for a map which key is the offset of a dwarf
> > +/// die, (given by dwarf_dieoffset()) and which value is the
> > +/// corresponding enum_type_decl_sptr.
> > +typedef unordered_map<Dwarf_Off, enum_type_decl_sptr> die_enum_map_type;
> > +
>
> Adding this typedef is unnecessary because ...
>
> [...]
>
> > @@ -2273,6 +2282,9 @@ public:
> >    die_class_or_union_map_type        die_wip_classes_map_;
> >    die_class_or_union_map_type        alternate_die_wip_classes_map_;
> >    die_class_or_union_map_type        type_unit_die_wip_classes_map_;
> > +  die_enum_map_type          die_wip_enums_map_;
> > +  die_enum_map_type          alternate_die_wip_enums_map_;
> > +  die_enum_map_type          type_unit_die_wip_enums_map_;
>
> ... these new data members are unnecessary.  Because enum types are
> built atomically, they don't have to go through a 'work-in-progress'
> process.  That process is only suited for classes and function types
> because these are built gradually and thus stay non-fully constructed
> for a certain time during which other types are built.  Hence these
> transiently non fully built types (a.k.a WIP types) are 'marked' as
> such.
>
> [...]
>
>
> > +  /// Getter of a map that associates a die that represents a
> > +  /// enum with the declaration of the enum, while the enum
> > +  /// is being constructed.
> > +  ///
> > +  /// @param source where the DIE is from.
> > +  ///
> > +  /// @return the map that associates a DIE to the enum that is being
> > +  /// built.
> > +  const die_enum_map_type&
> > +  die_wip_enums_map(die_source source) const
> > +  {return const_cast<read_context*>(this)->die_wip_enums_map(source);}
>
> This accessor is unnecessary as well.
>
> [...]
>
> > +
> > +  /// Getter of a map that associates a die that represents a
> > +  /// enum with the declaration of the enum, while the enum
> > +  /// is being constructed.
> > +  ///
> > +  /// @param source where the DIE comes from.
> > +  ///
> > +  /// @return the map that associates a DIE to the enum that is being
> > +  /// built.
> > +  die_enum_map_type&
> > +  die_wip_enums_map(die_source source)
>
> Likewise.
>
> [...]
>
> > @@ -16660,7 +16943,18 @@ build_ir_node_from_die(read_context& ctxt,
> >
> >      case DW_TAG_enumeration_type:
> >        {
> > -     if (!type_is_suppressed(ctxt, scope, die))
> > +     bool type_is_private = false;
> > +     bool type_suppressed =
> > +       type_is_suppressed(ctxt, scope, die, type_is_private);
> > +     if (type_suppressed && type_is_private)
> > +       // The type is suppressed because it's private.  If other
> > +       // non-suppressed and declaration-only instances of this
> > +       // type exist in the current corpus, then it means those
> > +       // non-suppressed instances are opaque versions of the
> > +       // suppressed private type.  Lets return one of these opaque
> > +       // types then.
> > +       result = get_opaque_version_of_type(ctxt, scope, die, where_offset);
>
> Right.  Though, for this to work, get_opaque_version_of_type must be
> adapted to function with enum types.  Note that at the moment, it only
> functions for classes (not even for unions).  I have added a patch to
> dodji/incomp-enums at
> https://sourceware.org/git/?p=libabigail.git;a=commit;h=4e361d914e8e6c34d5e00f570551b4b5a545fd82
> to do that.  That patch factorizes a some parts of build_enum_type
> because it needed it to reuse some them.
>
> > +     else if (!type_suppressed)
> >         {
>
> [...]
>
> > @@ -13386,18 +13634,32 @@ build_enum_type(read_context&       ctxt,
>
> >    if (tag != DW_TAG_enumeration_type)
> >      return result;
> >
> > +  die_source source;
> > +  ABG_ASSERT(ctxt.get_die_source(die, source));
> > +  {
> > +    die_enum_map_type::const_iterator i =
> > +      ctxt.die_wip_enums_map(source).find(dwarf_dieoffset(die));
> > +    if (i != ctxt.die_wip_enums_map(source).end())
> > +      {
> > +     enum_type_decl_sptr u = is_enum_type(i->second);
> > +     ABG_ASSERT(u);
> > +     return u;
> > +      }
> > +  }
>
> As I said earlier, this WIP handling is unnecessary.
>
> [...]
>
> > -  bool enum_is_anonymous = false;
> > +  bool is_anonymous = false;
>
> This change in unnecessary.
>
> >    // If the enum is anonymous, let's give it a name.
> >    if (name.empty())
> >      {
> >        name = get_internal_anonymous_die_prefix_name(die);
> >        ABG_ASSERT(!name.empty());
> >        // But we remember that the type is anonymous.
> > -      enum_is_anonymous = true;
> > +      is_anonymous = true;
>
> Likewise.
>
> >
> >        if (size_t s = scope->get_num_anonymous_member_enums())
> >       name = build_internal_anonymous_die_name(name, s);
> > @@ -13409,7 +13671,7 @@ build_enum_type(read_context& ctxt,
> >    // representation (name) and location can be later detected as being
> >    // for the same type.
> >
> > -  if (!enum_is_anonymous)
> > +  if (!is_anonymous)
>
> Likewise.
>
> >      {
> >        if (use_odr)
> >       {
> > @@ -13442,6 +13704,7 @@ build_enum_type(read_context& ctxt,
> >    uint64_t size = 0;
> >    if (die_unsigned_constant_attribute(die, DW_AT_byte_size, size))
> >      size *= 8;
> > +  bool is_artificial = die_is_artificial(die);
>
> This change is ...
>
> [...]
>
> >    // for now we consider that underlying types of enums are all anonymous
> >    bool enum_underlying_type_is_anonymous= true;
> > @@ -13474,8 +13737,6 @@ build_enum_type(read_context& ctxt,
> >        while (dwarf_siblingof(&child, &child) == 0);
> >      }
> >
> > -  bool is_artificial = die_is_artificial(die);
> > -
>
> ... unnecessary.
>
> >    // DWARF up to version 4 (at least) doesn't seem to carry the
> >    // underlying type, so let's create an artificial one here, which
> >    // sole purpose is to be passed to the constructor of the
> > @@ -13491,9 +13752,13 @@ build_enum_type(read_context&        ctxt,
> >    t = dynamic_pointer_cast<type_decl>(d);
> >    ABG_ASSERT(t);
> >    result.reset(new enum_type_decl(name, loc, t, enms, linkage_name));
> > -  result->set_is_anonymous(enum_is_anonymous);
> > +  result->set_is_anonymous(is_anonymous);
>
> Likewise.
>
> [...]
>
>
> > +++ b/src/abg-ir.cc
>
> [...]
>
>
> > +/// Constructor for instances of enum_type_decl that represent a
> > +/// declaration without definition.
> > +///
> > +/// @param env the environment we are operating from.
> > +///
> > +/// @param name the name of the enum.
> > +enum_type_decl::enum_type_decl(const environment*    env,
> > +                            const string&            name)
> > +  : type_or_decl_base(env,
> > +                   ENUM_TYPE
> > +                   | ABSTRACT_TYPE_BASE
> > +                   | ABSTRACT_DECL_BASE),
> > +    type_base(env, 0, 0),
> > +    decl_base(env, name, location(), name),
> > +    priv_(new priv)
> > +{
> > +  runtime_type_instance(this);
> > +}
> > +
>
> This constructor is unnecessary.
>
> [...]
>
>
> > +/// If this @ref enum_type_decl_sptr is a definition, get its earlier
> > +/// declaration.
> > +///
> > +/// @return the earlier declaration of the enum, if any.
> > +decl_base_sptr
> > +enum_type_decl::get_earlier_declaration() const
> > +{return priv_->declaration_;}
>
> I think this is unnecessary.  Classes do have a similar accessor but
> it's a relique from ancient times when resolution of decl-only classes
> was done differently.  I keep it around to be able to support ancient
> abixml files that might be out there somewhere.
>
> > +
> > +/// set the earlier declaration of this @ref enum_type_decl definition.
> > +///
> > +/// @param declaration the earlier declaration to set.  Note that it's
> > +/// set only if it's a pure declaration.
> > +void
> > +enum_type_decl::set_earlier_declaration(decl_base_sptr declaration)
> > +{
> > +  enum_type_decl_sptr cl = dynamic_pointer_cast<enum_type_decl>(declaration);
> > +  if (cl && cl->get_is_declaration_only())
> > +    priv_->declaration_ = declaration;
> > +}
>
> Likewise.
>
> [...]
>
>
> Thanks again for working on this.
>
> Cheers,
>
> --
>                 Dodji