Fix meaning of "harmless name change" to avoid overfiltering

Whenever a typedef or enum changes its name, we seem to wrongly
qualify that chane as being "harmless" and thus, we tend to filter the
change out, by default.  We do this even when e.g, the enum change
also contain other changes that might be meaningful to show.

This commit fixes the issue by "tightening" the qualification of a
harmless name change for typedefs and enums.

For typedefs, a name change is harmless only if there is no change in
the textual representation of the underlying type.

For enums, a name change is harmless only if there is no other change
in the enum, like on the enumerators or on the underlying type of the
enum.

This patch is part of the set of patches whose titles are:

    Do not show decl-only-to-def changes in the leaf reporter
    Overhaul of the report diff stats summary
    Do not mark "distinct" diff nodes as being redundant
    Fix meaning of "harmless name change" to avoid overfiltering
    Better handle category propagation of pointer changes
    Improve function changes reporting in leaf and default mode
    Don't filter out typedef changes with redundant underlying type changes
    Only show leaf type changes in the leaf type changes section
    Fix leaf report of class data member changes
    Always show redundant changes in leaf mode
    Avoid reporting an enum change if it has already been reported
    When we say an a change was reported earlier give its source location
    [abipkgdiff]: in leaf mode we always show redundant changes
    Update tests for the "better leaf mode redundancy management" patchset

	* include/abg-ir.h (enum_has_non_name_change): Declare new
	* function.  Make it a friend of class enum_type_decl.
	* src/abg-comp-filter.cc (has_harmless_name_change): A typedef
	 name change cannot be harmless if the textual representation of
	the underlying type changes too.  Also, use the new
	enum_has_non_name_change to tighten the harmless name change
	definition for an enum.
	* src/abg-default-reporter.cc
	(default_reporter::report_local_typedef_changes): If the name of
	the typedef changed, report it no matter what.
	* src/abg-ir.cc (enum_has_non_name_change): Define new function.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>

diff --git a/include/abg-ir.h b/include/abg-ir.h
index b7cc6db0aebb846b90e3be04665e5a741fda7ccd..47816d8c83fae872714c97409d39952376d5f875 100644 (file)
--- a/include/abg-ir.h
+++ b/include/abg-ir.h
@@ -2241,6 +2241,11 @@ public:
   traverse(ir_node_visitor& v);
 
   virtual ~enum_type_decl();
+
+  friend bool
+  enum_has_non_name_change(const enum_type_decl& l,
+                          const enum_type_decl& r,
+                          change_kind* k);
 }; // end class enum_type_decl
 
 bool
@@ -2249,6 +2254,11 @@ operator==(const enum_type_decl_sptr& l, const enum_type_decl_sptr& r);
 bool
 operator!=(const enum_type_decl_sptr& l, const enum_type_decl_sptr& r);
 
+bool
+enum_has_non_name_change(const enum_type_decl& l,
+                        const enum_type_decl& r,
+                        change_kind* k);
+
 /// The abstraction of an enumerator
 class enum_type_decl::enumerator
 {

diff --git a/src/abg-comp-filter.cc b/src/abg-comp-filter.cc
index 2a16bb00af897a1d430c5854bb6b38e38bbd096a..0398b924440e586bc83481e6500f166756f5ddad 100644 (file)
--- a/src/abg-comp-filter.cc
+++ b/src/abg-comp-filter.cc
@@ -407,8 +407,8 @@ decl_name_changed(const diff *d)
 
 /// Test if two decls represents a harmless name change.
 ///
-/// For now, a harmless name change is a name change for a typedef,
-/// enum or data member.
+/// For now, a harmless name change is considered only for a typedef,
+/// enum or a data member.
 ///
 /// @param f the first decl to consider in the comparison.
 ///
@@ -418,10 +418,27 @@ decl_name_changed(const diff *d)
 bool
 has_harmless_name_change(const decl_base_sptr& f, const decl_base_sptr& s)
 {
+  // So, a harmless name change is either ...
   return (decl_name_changed(f, s)
-         && ((is_typedef(f) && is_typedef(s))
-             || (is_data_member(f) && is_data_member(s))
-             || (is_enum_type(f) && is_enum_type(s))));
+         && (// ... a typedef name change, without having the
+             // underlying type changed ...
+             (is_typedef(f)
+              && is_typedef(s)
+              && (is_typedef(f)->get_underlying_type()
+                  == is_typedef(s)->get_underlying_type()))
+             // .. or a data member name change, without having the
+             // its type changed ...
+             || (is_data_member(f)
+                 && is_data_member(s)
+                 && (is_var_decl(f)->get_type()
+                     == is_var_decl(s)->get_type()))
+             // .. an enum name change without having any other part
+             // of the enum to change.
+             || (is_enum_type(f)
+                 && is_enum_type(s)
+                 && !enum_has_non_name_change(*is_enum_type(f),
+                                              *is_enum_type(s),
+                                              0))));
 }
 
 /// Test if a class_diff node has non-static members added or

diff --git a/src/abg-default-reporter.cc b/src/abg-default-reporter.cc
index db5666878acd247df445760c5c02dc7d332369cd..5769d1cc1511ad415b007d8ea126e58802218e9d 100644 (file)
--- a/src/abg-default-reporter.cc
+++ b/src/abg-default-reporter.cc
@@ -210,11 +210,11 @@ default_reporter::report_local_typedef_changes(const typedef_diff &d,
 
   maybe_report_diff_for_member(f, s, d.context(), out, indent);
 
-  if (filtering::has_harmless_name_change(f, s)
-      && ((d.context()->get_allowed_category()
-          & HARMLESS_DECL_NAME_CHANGE_CATEGORY)
-         ||
-         d.context()->show_leaf_changes_only()))
+  if ((filtering::has_harmless_name_change(f, s)
+       && ((d.context()->get_allowed_category()
+           & HARMLESS_DECL_NAME_CHANGE_CATEGORY)
+          || d.context()->show_leaf_changes_only()))
+      || f->get_qualified_name() != s->get_qualified_name())
     {
       out << indent << "typedef name changed from "
          << f->get_qualified_name()

diff --git a/src/abg-ir.cc b/src/abg-ir.cc
index dd7ddf0dd21f0cfa3c3f407e9c8fc10a8c47700f..a2fd48f1733b6de2de268db79db0c0eff06198e8 100644 (file)
--- a/src/abg-ir.cc
+++ b/src/abg-ir.cc
@@ -12927,6 +12927,80 @@ enum_type_decl::traverse(ir_node_visitor &v)
 enum_type_decl::~enum_type_decl()
 {}
 
+/// Test if two enums differ, but not by a name change.
+///
+/// @param l the first enum to consider.
+///
+/// @param r the second enum to consider.
+///
+/// @return true iff @p l differs from @p r by anything but a name
+/// change.
+bool
+enum_has_non_name_change(const enum_type_decl& l,
+                        const enum_type_decl& r,
+                        change_kind* k)
+{
+  bool result = false;
+  if (*l.get_underlying_type() != *r.get_underlying_type())
+    {
+      result = true;
+      if (k)
+       *k |= SUBTYPE_CHANGE_KIND;
+      else
+       return true;
+    }
+
+  enum_type_decl::enumerators::const_iterator i, j;
+  for (i = l.get_enumerators().begin(), j = r.get_enumerators().begin();
+       i != l.get_enumerators().end() && j != r.get_enumerators().end();
+       ++i, ++j)
+    if (*i != *j)
+      {
+       result = true;
+       if (k)
+         {
+           *k |= LOCAL_CHANGE_KIND;
+           break;
+         }
+       else
+         return true;
+      }
+
+  if (i != l.get_enumerators().end() || j != r.get_enumerators().end())
+    {
+      result = true;
+      if (k)
+       *k |= LOCAL_CHANGE_KIND;
+      else
+       return true;
+    }
+
+  enum_type_decl &local_r = const_cast<enum_type_decl&>(r);
+  interned_string qn_r = l.get_environment()->intern(r.get_qualified_name());
+  interned_string qn_l = l.get_environment()->intern(l.get_qualified_name());
+  string n_l = l.get_name();
+  string n_r = r.get_name();
+  local_r.set_qualified_name(qn_l);
+  local_r.set_name(n_l);
+
+  if (!(l.decl_base::operator==(r) && l.type_base::operator==(r)))
+    {
+      result = true;
+      if (k)
+       *k |= LOCAL_CHANGE_KIND;
+      else
+       {
+         local_r.set_name(n_r);
+         local_r.set_qualified_name(qn_r);
+         return true;
+       }
+    }
+  local_r.set_qualified_name(qn_r);
+  local_r.set_name(n_r);
+
+  return result;
+}
+
 /// Compares two instances of @ref enum_type_decl.
 ///
 /// If the two intances are different, set a bitfield to give some