diff mbox series

[1/3] {default, leaf}-reporter: group data members changes reports together

Message ID 86imgta2f6.fsf@seketeli.org
State Committed
Headers show
Series [1/3] {default, leaf}-reporter: group data members changes reports together | expand

Commit Message

Dodji Seketeli May 18, 2020, 11:49 a.m. UTC
Hello,

There are two kinds of data member changes:

  1/ changes to the type or offset of a given data member
     basically, in this kind of change, the name of the data member
     remained the same.

  2/ changes where the data member (at a given offset) was replaced by
     something else completely.

Today, the comparison engine recognizes these two kinds of changes and
records them in two different data structures.  This is useful because
it allows for a finer grain analysis.

But when we report these changes, today, we report them separately and
sometimes that doesn't make sense.  For instance, because there is no
change of the 1/ kind, the reporter would say "no data member
changes", and yet go ahead and emit data member changes of the 2/ kind.

This patch groups the reporting of the two kinds of changes so that
when it says "no data member changes", it means there was no data
member changes at all.

	* include/abg-comparison.h
	(class_or_union_diff::{sorted_changed_data_members,
	count_filtered_changed_data_members,
	sorted_subtype_changed_data_members,
	count_filtered_subtype_changed_data_members}): Declare ...
	* src/abg-comparison.cc
	(class_or_union_diff::{sorted_changed_data_members,
	count_filtered_changed_data_members,
	sorted_subtype_changed_data_members,
	count_filtered_subtype_changed_data_members}): ... accessors for
	existing private data members.
	* src/abg-default-reporter.cc (default_reporter::report): In the
	class_or_union_diff& overload, group the reporting of the changes
	to data member sub-types with the replacement of data members.
	These are just data member changes after all.  Use the newly
	declared accessors for better measure.
	* src/abg-leaf-reporter.cc (leaf_reporter::report): Likewise.
	* tests/data/test-diff-dwarf/test45-anon-dm-change-report-0.txt: Adjust.
	* src/abg-leaf-reporter.cc (leaf_reporter::report): Likewise.
	* tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt:
	Likewise.

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

Applied to master.

---
 include/abg-comparison.h                           | 12 +++++
 src/abg-comparison.cc                              | 30 ++++++++++++
 src/abg-default-reporter.cc                        | 51 ++++++++------------
 src/abg-leaf-reporter.cc                           | 55 ++++++----------------
 .../test45-anon-dm-change-report-0.txt             |  5 +-
 ...-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt |  9 ++--
 6 files changed, 82 insertions(+), 80 deletions(-)
diff mbox series

Patch

diff --git a/include/abg-comparison.h b/include/abg-comparison.h
index 4f60ff9..cf95624 100644
--- a/include/abg-comparison.h
+++ b/include/abg-comparison.h
@@ -1598,6 +1598,18 @@  public:
   const string_member_function_sptr_map&
   inserted_member_fns() const;
 
+  const var_diff_sptrs_type&
+  sorted_changed_data_members() const;
+
+  size_t
+  count_filtered_changed_data_members(bool local_only = false) const;
+
+  const var_diff_sptrs_type&
+  sorted_subtype_changed_data_members() const;
+
+  size_t
+  count_filtered_subtype_changed_data_members(bool local_only = false) const;
+
   const edit_script&
   member_fn_tmpls_changes() const;
 
diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index 57cc6c2..e4a983b 100644
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -4955,6 +4955,36 @@  const string_member_function_sptr_map&
 class_or_union_diff::inserted_member_fns() const
 {return get_priv()->inserted_member_functions_;}
 
+/// Getter of the sorted vector of data members that got replaced by
+/// another data member.
+///
+/// @return sorted vector of changed data member.
+const var_diff_sptrs_type&
+class_or_union_diff::sorted_changed_data_members() const
+{return get_priv()->sorted_changed_dm_;}
+
+/// Count the number of /filtered/ data members that got replaced by
+/// another data member.
+///
+/// @return the number of changed data member that got filtered out.
+size_t
+class_or_union_diff::count_filtered_changed_data_members(bool local) const
+{return get_priv()->count_filtered_changed_dm(local);}
+
+/// Getter of the sorted vector of data members with a (sub-)type change.
+///
+/// @return sorted vector of changed data member.
+const var_diff_sptrs_type&
+class_or_union_diff::sorted_subtype_changed_data_members() const
+{return get_priv()->sorted_subtype_changed_dm_;}
+
+/// Count the number of /filtered/ data members with a sub-type change.
+///
+/// @return the number of changed data member that got filtered out.
+size_t
+class_or_union_diff::count_filtered_subtype_changed_data_members(bool local) const
+{return get_priv()->count_filtered_subtype_changed_dm(local);}
+
 /// @return the edit script of the member function templates of the two
 /// @ref class_or_union.
 const edit_script&
diff --git a/src/abg-default-reporter.cc b/src/abg-default-reporter.cc
index 5892bec..b59e8d6 100644
--- a/src/abg-default-reporter.cc
+++ b/src/abg-default-reporter.cc
@@ -989,43 +989,32 @@  default_reporter::report(const class_or_union_diff& d,
 	}
 
       // report change
-      size_t numchanges =
-	d.class_or_union_diff::get_priv()->sorted_subtype_changed_dm_.size();
-      size_t num_filtered =
-	d.class_or_union_diff::get_priv()->count_filtered_subtype_changed_dm();
-      if (numchanges)
+      size_t num_changes =
+	(d.sorted_subtype_changed_data_members().size()
+	 + d.sorted_changed_data_members().size());
+
+      size_t num_changes_filtered =
+	(d.count_filtered_subtype_changed_data_members()
+	 + d.count_filtered_changed_data_members());
+
+      if (num_changes)
 	{
-	  report_mem_header(out, numchanges, num_filtered,
-			    subtype_change_kind, "data member", indent);
+	  report_mem_header(out, num_changes, num_changes_filtered,
+			    change_kind, "data member", indent);
+
 	  for (var_diff_sptrs_type::const_iterator it =
-		 d.class_or_union_diff::get_priv()->sorted_subtype_changed_dm_.begin();
-	       it != d.class_or_union_diff::get_priv()->sorted_subtype_changed_dm_.end();
+		 d.sorted_changed_data_members().begin();
+	       it != d.sorted_changed_data_members().end();
 	       ++it)
-	    {
-	      if ((*it)->to_be_reported())
-		{
-		  represent(*it, ctxt, out, indent + "  ");
-		}
-	    }
-	}
+	    if ((*it)->to_be_reported())
+	      represent(*it, ctxt, out, indent + "  ");
 
-      numchanges = d.class_or_union_diff::get_priv()->sorted_changed_dm_.size();
-      num_filtered =
-	d.class_or_union_diff::get_priv()->count_filtered_changed_dm();
-      if (numchanges)
-	{
-	  report_mem_header(out, numchanges, num_filtered,
-			    change_kind, "data member", indent);
 	  for (var_diff_sptrs_type::const_iterator it =
-		 d.class_or_union_diff::get_priv()->sorted_changed_dm_.begin();
-	       it != d.class_or_union_diff::get_priv()->sorted_changed_dm_.end();
+		 d.sorted_subtype_changed_data_members().begin();
+	       it != d.sorted_subtype_changed_data_members().end();
 	       ++it)
-	    {
-	      if ((*it)->to_be_reported())
-		{
-		  represent(*it, ctxt, out, indent + "  ");
-		}
-	    }
+	    if ((*it)->to_be_reported())
+	      represent(*it, ctxt, out, indent + "  ");
 	}
     }
 
diff --git a/src/abg-leaf-reporter.cc b/src/abg-leaf-reporter.cc
index 16e8f36..afe5692 100644
--- a/src/abg-leaf-reporter.cc
+++ b/src/abg-leaf-reporter.cc
@@ -603,58 +603,33 @@  leaf_reporter::report(const class_or_union_diff& d,
 	}
 
       // report changes
-      size_t numchanges =
-	d.class_or_union_diff::get_priv()->sorted_changed_dm_.size();
+      size_t numchanges = (d.sorted_changed_data_members().size()
+			   + d.sorted_subtype_changed_data_members().size());
+
       size_t num_filtered =
-	d.class_or_union_diff::get_priv()->
-	count_filtered_changed_dm(/*local_only =*/ true);
+	(d.count_filtered_changed_data_members(/*local_only=*/true)
+	 + d.count_filtered_subtype_changed_data_members(/*local_only=*/true));
+
       ABG_ASSERT(numchanges >= num_filtered);
       size_t net_numchanges = numchanges - num_filtered;
 
-      bool emitted_data_members_changes = false;
       if (net_numchanges)
 	{
 	  report_mem_header(out, change_kind, "data member", indent);
+
 	  for (var_diff_sptrs_type::const_iterator it =
-		 d.class_or_union_diff::get_priv()->
-		 sorted_changed_dm_.begin();
-	       it != d.class_or_union_diff::get_priv()->
-		 sorted_changed_dm_.end();
+		 d.sorted_changed_data_members().begin();
+	       it != d.sorted_changed_data_members().end();
 	       ++it)
-	    {
-	      if (diff_to_be_reported((*it).get()))
-		{
-		  represent(*it, ctxt, out, indent + "  ",
-			    /*local_only=*/true);
-		  emitted_data_members_changes = true;
-		}
-	    }
-	}
+	    if (diff_to_be_reported((*it).get()))
+	      represent(*it, ctxt, out, indent + "  ", /*local_only=*/true);
 
-      numchanges =
-	d.class_or_union_diff::get_priv()->sorted_subtype_changed_dm_.size();
-      num_filtered =
-	d.class_or_union_diff::get_priv()->
-	count_filtered_subtype_changed_dm(/*local_only =*/ true);
-      ABG_ASSERT(numchanges >= num_filtered);
-      net_numchanges = numchanges - num_filtered;
-
-      if (net_numchanges)
-	{
-	  if (!emitted_data_members_changes)
-	    report_mem_header(out, subtype_change_kind, "data member", indent);
 	  for (var_diff_sptrs_type::const_iterator it =
-		 d.class_or_union_diff::get_priv()->sorted_subtype_changed_dm_.begin();
-	       it != d.class_or_union_diff::get_priv()->sorted_subtype_changed_dm_.end();
+		 d.sorted_subtype_changed_data_members().begin();
+	       it != d.sorted_subtype_changed_data_members().end();
 	       ++it)
-	    {
-	      if (diff_to_be_reported((*it).get()))
-		{
-		  represent(*it, ctxt, out, indent + "  ",
-			    /*local_only=*/true);
-		  emitted_data_members_changes = true;
-		}
-	    }
+	    if (diff_to_be_reported((*it).get()))
+	      represent(*it, ctxt, out, indent + "  ", /*local_only=*/true);
 	}
     }
 }
diff --git a/tests/data/test-diff-dwarf/test45-anon-dm-change-report-0.txt b/tests/data/test-diff-dwarf/test45-anon-dm-change-report-0.txt
index 2f8f31d..4ffd8b3 100644
--- a/tests/data/test-diff-dwarf/test45-anon-dm-change-report-0.txt
+++ b/tests/data/test-diff-dwarf/test45-anon-dm-change-report-0.txt
@@ -9,11 +9,10 @@  Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
         type size changed from 96 to 64 (in bits)
         1 data member insertion:
           'char S1::m01', at offset 32 (in bits)
-        1 data member change:
-          'char S1::m1' offset changed from 64 to 40 (in bits) (by -24 bits)
-        1 data member change:
+        2 data member changes:
           anonymous data member struct {int m0; char m01;} at offset 0 (in bits) became data member 'int S1::m0'
           and size changed from 64 to 32 (in bits) (by -32 bits)
+          'char S1::m1' offset changed from 64 to 40 (in bits) (by -24 bits)
 
   [C] 'function void foo(S0&)' has some indirect sub-type changes:
     parameter 1 of type 'S0&' has sub-type changes:
diff --git a/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt b/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt
index 58c94b7..c64da3b 100644
--- a/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt
+++ b/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt
@@ -159,11 +159,11 @@ 
         in pointed to type 'typedef SpiceServer' at spice-server.h:38:1:
           underlying type 'struct RedsState' at reds-private.h:127:1 changed:
             type size hasn't changed
-            2 data member changes (1 filtered):
+            2 data member changes (2 filtered):
               type of 'VDIPortState RedsState::agent_state' changed:
                 underlying type 'struct VDIPortState' at reds-private.h:46:1 changed:
                   type size hasn't changed
-                  1 data member change:
+                  1 data member changes (3 filtered):
                     type of 'SpiceCharDeviceState* VDIPortState::base' changed:
                       in pointed to type 'typedef SpiceCharDeviceState' at spice-char.h:34:1:
                         underlying type 'struct SpiceCharDeviceState' at char_device.c:47:1 changed:
@@ -199,7 +199,6 @@ 
                                         pointed to type 'typedef SpiceCharDeviceInstance' changed at spice.h:399:1, as reported earlier
                             and offset changed from 1024 to 1088 (in bits) (by +64 bits)
                             'void* SpiceCharDeviceState::opaque' offset changed from 1472 to 1536 (in bits) (by +64 bits)
-                  no data member changes (3 filtered);
               type of 'MainChannel* RedsState::main_channel' changed:
                 in pointed to type 'typedef MainChannel' at main_channel.h:48:1:
                   underlying type 'struct MainChannel' at main_channel.h:36:1 changed:
@@ -404,7 +403,6 @@ 
                                       in pointed to type 'function type void (RedChannelClient*)':
                                         parameter 1 of type 'RedChannelClient*' has sub-type changes:
                                           pointed to type 'typedef RedChannelClient' changed at red_channel.h:136:1, as reported earlier
-            no data member change (1 filtered);
 
     [C] 'function int spice_server_add_interface(SpiceServer*, SpiceBaseInstance*)' at reds.c:3159:1 has some indirect sub-type changes:
       parameter 1 of type 'SpiceServer*' has sub-type changes:
@@ -519,7 +517,7 @@ 
                             in pointed to type 'typedef SndChannel' at snd_worker.c:74:1:
                               underlying type 'struct SndChannel' at snd_worker.c:89:1 changed:
                                 type size hasn't changed
-                                6 data member changes:
+                                6 data member changes (1 filtered):
                                   type of 'RedsStream* SndChannel::stream' changed:
                                     pointed to type 'typedef RedsStream' changed at red_channel.h:134:1, as reported earlier
                                   type of 'SndWorker* SndChannel::worker' changed:
@@ -545,7 +543,6 @@ 
                                         parameter 1 of type 'SndChannel*' has sub-type changes:
                                           in pointed to type 'typedef SndChannel' at snd_worker.c:74:1:
                                             underlying type 'struct SndChannel' changed, as being reported
-                                no data member change (1 filtered);
                           type of 'SndWorker* SndWorker::next' changed:
                             in pointed to type 'typedef SndWorker' at snd_worker.c:80:1:
                               underlying type 'struct SndWorker' changed, as being reported