[v3,1/1] Fix has_net_changes for --leaf-changes-only mode

  This function was not aware of --leaf-changes-only mode.

    - Stats counters for changed variables and types have
      different names in the different modes.
    - Net leaf type changes were not included in leaf mode.

For some inputs, this resulted in abidiff producing an empty report
but returning a non-zero exit status in --leaf-changes-only mode.

For other inputs the combination of both issues still resulted in the
correct return code. This included the following test-abidiff-exit
test cases:

    - test-leaf-peeling
    - test-leaf2
    - test-no-stray-comma

This patch makes has_net_changes mirror emit_diff_stats, modulo flags
like --non-reachable-types which if absent can still result in
discrepancies between output and return code.

The tests below verify that the exit code is zero when all the changes
between the test files are suppressed.

	* src/abg-comparison.cc (corpus_diff::has_net_changes):
	Reorder the logic to match emit_diff_stats, add a note about
	this; in --leaf-changes-only mode use the right counters for
	function and variable changes and include type changes.
	* tests/data/Makefile.am: Add new test case files.
	* tests/data/test-abidiff-exit/test-net-change-report0.txt:
	Normal mode, nothing suppressed.
	* tests/data/test-abidiff-exit/test-net-change-report1.txt:
	Normal mode, everything suppressed.
	* tests/data/test-abidiff-exit/test-net-change-report2.txt:
	Leaf mode, nothing suppressed.
	* tests/data/test-abidiff-exit/test-net-change-report3.txt:
	Leaf mode, everything suppressions.
	* tests/data/test-abidiff-exit/test-net-change-v0.c: Test file
	* tests/data/test-abidiff-exit/test-net-change-v0.o: Test file
	* tests/data/test-abidiff-exit/test-net-change-v1.c: Test file
	* tests/data/test-abidiff-exit/test-net-change-v1.o: Test file
	* tests/data/test-abidiff-exit/test-net-change.abignore: This
	suppresses changes for all variables, functions and types in
	the test files, except for the 'victim' function.
	* tests/test-abidiff-exit.cc: Run new test cases.

Reviewed-by: Matthias Maennich <maennich@google.com>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 src/abg-comparison.cc                         |  26 +++++++----
 tests/data/Makefile.am                        |   9 ++++
 .../test-net-change-report0.txt               |  43 ++++++++++++++++++
 .../test-net-change-report1.txt               |   3 ++
 .../test-net-change-report2.txt               |  42 +++++++++++++++++
 .../test-net-change-report3.txt               |   5 ++
 .../test-abidiff-exit/test-net-change-v0.c    |  14 ++++++
 .../test-abidiff-exit/test-net-change-v0.o    | Bin 0 -> 3512 bytes
 .../test-abidiff-exit/test-net-change-v1.c    |  14 ++++++
 .../test-abidiff-exit/test-net-change-v1.o    | Bin 0 -> 3552 bytes
 .../test-net-change.abignore                  |   8 ++++
 tests/test-abidiff-exit.cc                    |  38 ++++++++++++++++
 12 files changed, 192 insertions(+), 10 deletions(-)
 create mode 100644 tests/data/test-abidiff-exit/test-net-change-report0.txt
 create mode 100644 tests/data/test-abidiff-exit/test-net-change-report1.txt
 create mode 100644 tests/data/test-abidiff-exit/test-net-change-report2.txt
 create mode 100644 tests/data/test-abidiff-exit/test-net-change-report3.txt
 create mode 100644 tests/data/test-abidiff-exit/test-net-change-v0.c
 create mode 100644 tests/data/test-abidiff-exit/test-net-change-v0.o
 create mode 100644 tests/data/test-abidiff-exit/test-net-change-v1.c
 create mode 100644 tests/data/test-abidiff-exit/test-net-change-v1.o
 create mode 100644 tests/data/test-abidiff-exit/test-net-change.abignore
  

Hello Giuliano,

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

[...]

> diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
> index c1d86a7f..e4b86426 100644
> --- a/src/abg-comparison.cc
> +++ b/src/abg-comparison.cc
> @@ -10847,21 +10847,27 @@ corpus_diff::has_net_changes() const
>      const diff_stats& stats = const_cast<corpus_diff*>(this)->
>        apply_filters_and_suppressions_before_reporting();
>  
> +    // Logic here should match emit_diff_stats.
> +    // TODO: Possibly suppress things that won't be shown there.
> +    bool leaf = context()->show_leaf_changes_only();

I think that right here, we shouldn't be making assumptions about the
kind of reporter (leaf or default) we are using.  This is a relic from
the ancient time predating the support of multiple reporters.  I (maybe
too) progressively started to delegate reporter-specific behaviour to
the reporters, especially with how corpus_diff::has_net_changes is
implemented.

I think we should move further into that direction, to keep a more
maintainable code base.  In practise, that means that this comparison
engine should remain as uncoupled from the reporters as possible .

So I have amended your patch a little bit to achieve that, in the same
spirit as what has been done for corpus_diff::has_net_changes.

Please find below the amended patch I am proposing to apply instead.
The tests are all the same and pass distcheck.

From 76e53729f426e620268ebb43746476684112023d Mon Sep 17 00:00:00 2001
From: Giuliano Procida <gprocida@google.com>
Date: Fri, 17 Jul 2020 14:01:01 +0200
Subject: [PATCH] Fix corpus_diff::has_net_changes for --leaf-changes-only mode

This function was not aware of --leaf-changes-only mode.

    - Stats counters for changed variables and types have
      different names in the different modes.
    - Net leaf type changes were not included in leaf mode.

For some inputs, this resulted in abidiff producing an empty report
but returning a non-zero exit status in --leaf-changes-only mode.

For other inputs the combination of both issues still resulted in the
correct return code. This included the following test-abidiff-exit
test cases:

    - test-leaf-peeling
    - test-leaf2
    - test-no-stray-comma

This patch makes corpus_diff::has_net_changes mirror emit_diff_stats,
modulo flags like --non-reachable-types which if absent can still
result in discrepancies between output and return code.

To achieve this in a more maintainable way, the patch introduces a new interface
reporter_base::diff_has_net_changes.  That interface is implemented by
all current reporters.  Each reporter focuses on its own
particularities to provide the required behavious. Then
corpus_diff:has_net_changes just has to invoke
reporter_base::diff_has_net_changes on the reporter that is currently
in used.

The tests below verify that the exit code is zero when all the changes
between the test files are suppressed.

	* include/abg-reporter.h ({reporter_base, default_reporter,
	leaf_reporter}::diff_has_net_changes): Add new virtual function.
	This breaks binary compatibility but should conserve source
	compatibility.
	* src/abg-default-reporter.cc
	(default_reporter::diff_has_net_changes): Define new member
	function.
	* src/abg-leaf-reporter.cc (leaf_reporter::diff_has_net_changes):
	Likewise.
	* src/abg-comparison.cc (corpus_diff::has_net_changes): Invoke
	reporter_base::diff_has_net_changes on the current reporter,
	rather than trying to handle all the different kinds of reporters
	here.
	(corpus_diff::priv::apply_filters_and_compute_diff_stats): Add a
	TODO to possibly delegate the implementation of this function to
	the reporters.
	* tests/data/Makefile.am: Add new test case files.
	* tests/data/test-abidiff-exit/test-net-change-report0.txt:
	Normal mode, nothing suppressed.
	* tests/data/test-abidiff-exit/test-net-change-report1.txt:
	Normal mode, everything suppressed.
	* tests/data/test-abidiff-exit/test-net-change-report2.txt:
	Leaf mode, nothing suppressed.
	* tests/data/test-abidiff-exit/test-net-change-report3.txt:
	Leaf mode, everything suppressions.
	* tests/data/test-abidiff-exit/test-net-change-v0.c: Test file
	* tests/data/test-abidiff-exit/test-net-change-v0.o: Test file
	* tests/data/test-abidiff-exit/test-net-change-v1.c: Test file
	* tests/data/test-abidiff-exit/test-net-change-v1.o: Test file
	* tests/data/test-abidiff-exit/test-net-change.abignore: This
	suppresses changes for all variables, functions and types in
	the test files, except for the 'victim' function.
	* tests/test-abidiff-exit.cc: Run new test cases.

Reviewed-by: Matthias Maennich <maennich@google.com>
Signed-off-by: Giuliano Procida <gprocida@google.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 include/abg-reporter.h                             |   6 +++
 src/abg-comparison.cc                              |  24 ++----------
 src/abg-default-reporter.cc                        |  35 +++++++++++++++++
 src/abg-leaf-reporter.cc                           |  36 +++++++++++++++++
 tests/data/Makefile.am                             |   9 +++++
 .../test-abidiff-exit/test-net-change-report0.txt  |  43 +++++++++++++++++++++
 .../test-abidiff-exit/test-net-change-report1.txt  |   3 ++
 .../test-abidiff-exit/test-net-change-report2.txt  |  42 ++++++++++++++++++++
 .../test-abidiff-exit/test-net-change-report3.txt  |   5 +++
 tests/data/test-abidiff-exit/test-net-change-v0.c  |  14 +++++++
 tests/data/test-abidiff-exit/test-net-change-v0.o  | Bin 0 -> 3512 bytes
 tests/data/test-abidiff-exit/test-net-change-v1.c  |  14 +++++++
 tests/data/test-abidiff-exit/test-net-change-v1.o  | Bin 0 -> 3552 bytes
 .../test-abidiff-exit/test-net-change.abignore     |   8 ++++
 tests/test-abidiff-exit.cc                         |  38 ++++++++++++++++++
 15 files changed, 257 insertions(+), 20 deletions(-)
 create mode 100644 tests/data/test-abidiff-exit/test-net-change-report0.txt
 create mode 100644 tests/data/test-abidiff-exit/test-net-change-report1.txt
 create mode 100644 tests/data/test-abidiff-exit/test-net-change-report2.txt
 create mode 100644 tests/data/test-abidiff-exit/test-net-change-report3.txt
 create mode 100644 tests/data/test-abidiff-exit/test-net-change-v0.c
 create mode 100644 tests/data/test-abidiff-exit/test-net-change-v0.o
 create mode 100644 tests/data/test-abidiff-exit/test-net-change-v1.c
 create mode 100644 tests/data/test-abidiff-exit/test-net-change-v1.o
 create mode 100644 tests/data/test-abidiff-exit/test-net-change.abignore

diff --git a/include/abg-reporter.h b/include/abg-reporter.h
index e0d9e66..bf113f0 100644
--- a/include/abg-reporter.h
+++ b/include/abg-reporter.h
@@ -73,6 +73,8 @@ public:
 
   virtual bool diff_to_be_reported(const diff *d) const;
 
+  virtual bool diff_has_net_changes(const corpus_diff *d) const = 0;
+
   virtual void
   report(const type_decl_diff& d, std::ostream& out,
 	 const std::string& indent = "") const = 0;
@@ -162,6 +164,8 @@ class default_reporter : public reporter_base
 {
 public:
 
+  virtual bool diff_has_net_changes(const corpus_diff *d) const;
+
   virtual void
   report(const type_decl_diff& d, std::ostream& out,
 	 const std::string& indent = "") const;
@@ -266,6 +270,8 @@ public:
 
   virtual bool diff_to_be_reported(const diff *d) const;
 
+  virtual bool diff_has_net_changes(const corpus_diff *d) const;
+
   void
   report_changes_from_diff_maps(const diff_maps&, std::ostream& out,
 				const std::string& indent) const;
diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index b6f065f..a9ef161 100644
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -10063,6 +10063,9 @@ corpus_diff::priv::apply_filters_and_compute_diff_stats(diff_stats& stat)
 /// Emit the summary of the functions & variables that got
 /// removed/changed/added.
 ///
+/// TODO: This should be handled by the reporters, just like what is
+/// done for reporter_base::diff_to_be_reported.
+///
 /// @param out the output stream to emit the stats to.
 ///
 /// @param indent the indentation string to use in the summary.
@@ -10848,26 +10851,7 @@ corpus_diff::has_net_subtype_changes() const
 /// carries subtype changes that are deemed incompatible ABI changes.
 bool
 corpus_diff::has_net_changes() const
-{
-    const diff_stats& stats = const_cast<corpus_diff*>(this)->
-      apply_filters_and_suppressions_before_reporting();
-
-    return (architecture_changed()
-	    || soname_changed()
-	    || stats.net_num_func_changed()
-	    || stats.net_num_vars_changed()
-	    || stats.net_num_func_added()
-	    || stats.net_num_added_func_syms()
-	    || stats.net_num_func_removed()
-	    || stats.net_num_removed_func_syms()
-	    || stats.net_num_vars_added()
-	    || stats.net_num_added_var_syms()
-	    || stats.net_num_vars_removed()
-	    || stats.net_num_removed_var_syms()
-	    || stats.net_num_added_unreachable_types()
-	    || stats.net_num_removed_unreachable_types()
-	    || stats.net_num_changed_unreachable_types());
-}
+{return  context()->get_reporter()->diff_has_net_changes(this);}
 
 /// Apply the different filters that are registered to be applied to
 /// the diff tree; that includes the categorization filters.  Also,
diff --git a/src/abg-default-reporter.cc b/src/abg-default-reporter.cc
index 2948e15..24a05fa 100644
--- a/src/abg-default-reporter.cc
+++ b/src/abg-default-reporter.cc
@@ -35,6 +35,41 @@ namespace abigail
 namespace comparison
 {
 
+/// Test if a given instance of @ref corpus_diff carries changes whose
+/// reports are not suppressed by any suppression specification.  In
+/// effect, these are deemed incompatible ABI changes.
+///
+/// @param d the @ref corpus_diff to consider
+///
+/// @return true iff @p d carries subtype changes that are deemed
+/// incompatible ABI changes.
+bool
+default_reporter::diff_has_net_changes(const corpus_diff *d) const
+{
+  if (!d)
+    return false;
+
+  const corpus_diff::diff_stats& stats = const_cast<corpus_diff*>(d)->
+    apply_filters_and_suppressions_before_reporting();
+
+  // Logic here should match emit_diff_stats.
+  return (d->architecture_changed()
+	  ||d->soname_changed()
+	  || stats.net_num_func_removed()
+	  || stats.net_num_func_changed()
+	  || stats.net_num_func_added()
+	  || stats.net_num_vars_removed()
+	  || stats.net_num_vars_changed()
+	  || stats.net_num_vars_added()
+	  || stats.net_num_removed_unreachable_types()
+	  || stats.net_num_changed_unreachable_types()
+	  || stats.net_num_added_unreachable_types()
+	  || stats.net_num_removed_func_syms()
+	  || stats.net_num_added_func_syms()
+	  || stats.net_num_removed_var_syms()
+	  || stats.net_num_added_var_syms());
+}
+
 /// Ouputs a report of the differences between of the two type_decl
 /// involved in the @ref type_decl_diff.
 ///
diff --git a/src/abg-leaf-reporter.cc b/src/abg-leaf-reporter.cc
index 783ffbc..0875c96 100644
--- a/src/abg-leaf-reporter.cc
+++ b/src/abg-leaf-reporter.cc
@@ -45,6 +45,42 @@ bool
 leaf_reporter::diff_to_be_reported(const diff *d) const
 {return d && d->to_be_reported() && d->has_local_changes();}
 
+/// Test if a given instance of @ref corpus_diff carries changes whose
+/// reports are not suppressed by any suppression specification.  In
+/// effect, these are deemed incompatible ABI changes.
+///
+/// @param d the @ref corpus_diff to consider
+///
+/// @return true iff @p d carries subtype changes that are deemed
+/// incompatible ABI changes.
+bool
+leaf_reporter::diff_has_net_changes(const corpus_diff *d) const
+{
+  if (!d)
+    return false;
+
+  const corpus_diff::diff_stats& stats = const_cast<corpus_diff*>(d)->
+    apply_filters_and_suppressions_before_reporting();
+
+  // Logic here should match emit_diff_stats.
+  return (d->architecture_changed()
+	  || d->soname_changed()
+	  || stats.net_num_func_removed()
+	  || stats.net_num_leaf_type_changes()
+	  || stats.net_num_leaf_func_changes()
+	  || stats.net_num_func_added()
+	  || stats.net_num_vars_removed()
+	  || stats.net_num_leaf_var_changes()
+	  || stats.net_num_vars_added()
+	  || stats.net_num_removed_unreachable_types()
+	  || stats.net_num_changed_unreachable_types()
+	  || stats.net_num_added_unreachable_types()
+	  || stats.net_num_removed_func_syms()
+	  || stats.net_num_added_func_syms()
+	  || stats.net_num_removed_var_syms()
+	  || stats.net_num_added_var_syms());
+}
+
 /// Report the changes carried by the diffs contained in an instance
 /// of @ref string_diff_ptr_map.
 ///
diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index 5da9ef9..ea94f0b 100644
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -165,6 +165,15 @@ test-abidiff-exit/test-decl-enum-v1.o \
 test-abidiff-exit/test-decl-enum-report.txt \
 test-abidiff-exit/test-decl-enum-report-2.txt \
 test-abidiff-exit/test-decl-enum-report-3.txt \
+test-abidiff-exit/test-net-change.abignore \
+test-abidiff-exit/test-net-change-v0.c \
+test-abidiff-exit/test-net-change-v0.o \
+test-abidiff-exit/test-net-change-v1.c \
+test-abidiff-exit/test-net-change-v1.o \
+test-abidiff-exit/test-net-change-report0.txt \
+test-abidiff-exit/test-net-change-report1.txt \
+test-abidiff-exit/test-net-change-report2.txt \
+test-abidiff-exit/test-net-change-report3.txt \
 \
 test-diff-dwarf/test0-v0.cc		\
 test-diff-dwarf/test0-v0.o			\
diff --git a/tests/data/test-abidiff-exit/test-net-change-report0.txt b/tests/data/test-abidiff-exit/test-net-change-report0.txt
new file mode 100644
index 0000000..66712b0
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-net-change-report0.txt
@@ -0,0 +1,43 @@
+Functions changes summary: 1 Removed, 2 Changed, 1 Added functions
+Variables changes summary: 1 Removed, 1 Changed, 1 Added variables
+
+1 Removed function:
+
+  [D] 'function int fun_removed()'    {fun_removed}
+
+1 Added function:
+
+  [A] 'function long int fun_added()'    {fun_added}
+
+2 functions with some indirect sub-type change:
+
+  [C] 'function int fun_changed()' has some indirect sub-type changes:
+    return type changed:
+      type name changed from 'int' to 'long int'
+      type size changed from 32 to 64 (in bits)
+
+  [C] 'function void victim(type_changed*)' has some indirect sub-type changes:
+    parameter 1 of type 'type_changed*' has sub-type changes:
+      in pointed to type 'struct type_changed':
+        type size changed from 32 to 64 (in bits)
+        1 data member change:
+          type of 'int type_changed::x' changed:
+            type name changed from 'int' to 'long int'
+            type size changed from 32 to 64 (in bits)
+
+1 Removed variable:
+
+  [D] 'int var_removed'    {var_removed}
+
+1 Added variable:
+
+  [A] 'long int var_added'    {var_added}
+
+1 Changed variable:
+
+  [C] 'int var_changed' was changed to 'long int var_changed':
+    size of symbol changed from 4 to 8
+    type of variable changed:
+      type name changed from 'int' to 'long int'
+      type size changed from 32 to 64 (in bits)
+
diff --git a/tests/data/test-abidiff-exit/test-net-change-report1.txt b/tests/data/test-abidiff-exit/test-net-change-report1.txt
new file mode 100644
index 0000000..4dd6096
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-net-change-report1.txt
@@ -0,0 +1,3 @@
+Functions changes summary: 0 Removed (1 filtered out), 0 Changed (2 filtered out), 0 Added (1 filtered out) functions
+Variables changes summary: 0 Removed (1 filtered out), 0 Changed (1 filtered out), 0 Added (1 filtered out) variables
+
diff --git a/tests/data/test-abidiff-exit/test-net-change-report2.txt b/tests/data/test-abidiff-exit/test-net-change-report2.txt
new file mode 100644
index 0000000..ca3b3e0
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-net-change-report2.txt
@@ -0,0 +1,42 @@
+Leaf changes summary: 7 artifacts changed
+Changed leaf types summary: 1 leaf type changed
+Removed/Changed/Added functions summary: 1 Removed, 1 Changed, 1 Added function
+Removed/Changed/Added variables summary: 1 Removed, 1 Changed, 1 Added variable
+
+1 Removed function:
+
+  [D] 'function int fun_removed()'    {fun_removed}
+
+1 Added function:
+
+  [A] 'function long int fun_added()'    {fun_added}
+
+1 function with some sub-type change:
+
+  [C] 'function int fun_changed()' has some sub-type changes:
+    return type changed:
+      type name changed from 'int' to 'long int'
+      type size changed from 32 to 64 (in bits)
+
+1 Removed variable:
+
+  [D] 'int var_removed'    {var_removed}
+
+1 Added variable:
+
+  [A] 'long int var_added'    {var_added}
+
+1 Changed variable:
+
+  [C] 'int var_changed' was changed to 'long int var_changed':
+    size of symbol changed from 4 to 8
+    type of variable changed:
+      type name changed from 'int' to 'long int'
+      type size changed from 32 to 64 (in bits)
+
+'struct type_changed' changed:
+  type size changed from 32 to 64 (in bits)
+  there are data member changes:
+    type 'int' of 'type_changed::x' changed:
+      type name changed from 'int' to 'long int'
+      type size changed from 32 to 64 (in bits)
diff --git a/tests/data/test-abidiff-exit/test-net-change-report3.txt b/tests/data/test-abidiff-exit/test-net-change-report3.txt
new file mode 100644
index 0000000..4856a77
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-net-change-report3.txt
@@ -0,0 +1,5 @@
+Leaf changes summary: 0 artifact changed (3 filtered out)
+Changed leaf types summary: 0 (1 filtered out) leaf type changed
+Removed/Changed/Added functions summary: 0 Removed (1 filtered out), 0 Changed (1 filtered out), 0 Added function (1 filtered out)
+Removed/Changed/Added variables summary: 0 Removed (1 filtered out), 0 Changed (1 filtered out), 0 Added variable (1 filtered out)
+
diff --git a/tests/data/test-abidiff-exit/test-net-change-v0.c b/tests/data/test-abidiff-exit/test-net-change-v0.c
new file mode 100644
index 0000000..684b9c6
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-net-change-v0.c
@@ -0,0 +1,14 @@
+int var_removed = 0;
+int fun_removed() { return 0; }
+
+int var_changed = 0;
+int fun_changed() { return 0; }
+
+struct type_removed {
+  int x;
+};
+struct type_changed {
+  int x;
+};
+
+void victim(struct type_changed * dummy) { (void)dummy->x; }
diff --git a/tests/data/test-abidiff-exit/test-net-change-v0.o b/tests/data/test-abidiff-exit/test-net-change-v0.o
new file mode 100644
index 0000000000000000000000000000000000000000..bd2c744744880e83ba295ba16995c32a7438ec53
GIT binary patch
literal 3512
zcmbtW&2Jk;6o2cD6Q^!mCoa$^MRE(M6uRp~R3%MGYY3zXA611^sS;G_+B>#a*t^ks
z9h_DOwL(a}6$z<D;#MSXh$FpK>Xkcx0US6W;8ei@-kY5lPbM32;7OkM=J$T@&CHvR
z^`~#WyPC5sV6osljCza$JSmLqx-{!hff=~6cIW3GsDJITVXocz<jLJ@kJ0rD{>F#{
z1)2Jww6*4#X6MMZto#b9Nt+$^SUw07NS$MbQ|2DC4ivVLo*#l$oS<lcacZoRRAd&Z
zL9xF67TXDFmYyR!_eBQ^h3jZdN;EHrAd_SimwI#ic)^;sUMN+Hc4@9OZOu%SpTW$h
z!7eQo&lS%W*DDuhb4LK|=b>0UU$n8LV-?FT6^p6gs^Ijez%DR-5eFW3tqZf9;lwPZ
ztslAf#w?L1@>*`nTsVZ8*U>ZS!YFFJvJ-`iIfXx2N6@wuF?|!TyCWZ<Wy8zZTGrpC
zDQx9&!JV`q5&gsoMdCC&Uf34S*1X%4f%ytdrh{O&l0W)(-nyEvEGXk8d(zDxzMZo%
zS!)n(#G)5%2_LqCW)k$EHt5H-Zq)R;wRRM>yQ0>KdZO0ejH71Ydo?fg<0$ZJ-Jsz$
zg0>fQYc!F5&G!;dnhxTA&}uni7$o2idcAGf^5Ttjnm#1kn_|?-S_Dmt*%vQesje)%
zT0QH&;?7r{UNQ*9a$AHV4w~5Y;$~-gxOm1nbGquZt4_=7Z->oJ9EH)K?+n6^gV1*p
zuhA8KISF1ZTwGaMs=oN9XaruECU+K3rUo3AWyBiacpt1`*_t~zQMijWKZ-d0NaV46
zy;S}@w=y-piUh>S_Zm4-_Va(3x-<RCwMXI5c!&S^XO9B`+{P>z@tq^E%WtD6OKkN@
zm_9qb$R><g<Q2}q<u%Q~WkoXZsXZ70Hk9_k_q<^6G%}~Pb*1kez;VQ9wEFN2T*YS|
z>m|i!9_v+w(>NrIXJ0`&>wL_1T`=tq=qFrVJj}~)<pIs6@p#3AQ*|E@h%%ijVEne~
zKV{(fCP|r9nof*<I!-E_^62jp)ro}j|CV;)oYIs>-@SJf&ip|2uWI~5$|mJ0oB0FP
zzhU6K{#y#?`SX6<H~8r@2=JYO-&6gEMjqbJ9}T|h!cPXDaecq?45BXY!>98_!t+mi
z099qsp6L5hH*lWkbpz)*fZN~h;bVt7iBsKSD-pv4TzaIzZS?!##-i)FzGw{E8$sBL
z&?jxL(TK&?F1Huc_holTHwXoX;1J+8qh3$ozXUgo65-;j?er6``H@N_IvcIn>j^dS
z|EcCMo@&VT=X7;t9AiLf;_5<v!pxV@(@C0b#-BhR-4iq3S26k>B8cA?<%R_`%$(%o
z_+Oc*w8=e%IOVJ>KdZ7T$oDFM&LQ!5KKLg^$}+ZQPI6pQ;5U*rYE15BbeR~j|5V`o
zC{$V|m)?o2`9D&h6pp9)6J*ZcN6gHL5FA&RXM;MLm|XH_jsLqEpX)^9(|c=<zlj*-
z<aweg<(3-%F9ww6@mb$QJS+aDI#AF4NBn0h-k`yK^FS&BkIVe8RJ{JykdNbsOjMfJ
z&+wOMWYw>rh-->S^(*0z<GHRhA9HL9DN|AuKEL#Rqwk9uPkWNJ)JN(Oeh>Y+nCqv!
QS@D<EKo9g_w9WW`0Y2;*ng9R*

literal 0
HcmV?d00001

diff --git a/tests/data/test-abidiff-exit/test-net-change-v1.c b/tests/data/test-abidiff-exit/test-net-change-v1.c
new file mode 100644
index 0000000..7b2a4ad
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-net-change-v1.c
@@ -0,0 +1,14 @@
+long var_added = 0;
+long fun_added() { return 0; }
+
+long var_changed = 0;
+long fun_changed() { return 0; }
+
+struct type_added {
+  long x;
+};
+struct type_changed {
+  long x;
+};
+
+void victim(struct type_changed * dummy) { (void)dummy->x; }
diff --git a/tests/data/test-abidiff-exit/test-net-change-v1.o b/tests/data/test-abidiff-exit/test-net-change-v1.o
new file mode 100644
index 0000000000000000000000000000000000000000..994ca58ecc5c35e7ecc51da87b685379c2734d6d
GIT binary patch
literal 3552
zcmbtW-D@0G6hE`mO*Y+Tn`EOV#KIImYO6DwV2NpJ+qKa&Q4uK=3mVedo!Q;N-I*{u
z8#fgZr3%HD3W5qEh=M+ekNVsfAAIo5zd%vnL=eFTJ?GB3vo|-BClAa$=lsss{l4?&
zGtaLV3<D$?a1lnDL;<Xckz5yI9jY(`*EVkd{4MEQcQtY2_8WiS*}5xy0e(S#M93wP
z{utvKJDEuU1(FS8Yz^6jDGg?+NMdR{q8UZ*l5w=Ohc3k-808X01B_8)R0JcjObwFp
z$(PuU3$gM5$%XejP%7O(NsQGfiW1~NG>S{Dg+r`hOdAhXs%5jXP?<Jn#;5MX%xA%@
ztd`H0&y_c;m*xv|0Oo^GE?-11oR+F#R?6knZd9@RlVFyl`wA8wbBs&#GQ+8PN?V`%
z@bWx4j~A8Pl(}>qGjF10!huoLW;Kh#!90eam`5nl*fLTVAp=o(89x&q#nv$XsZ3%k
zwhXSM0g2yF?9fl_X2%WNzP-ETG(~K21}4%Xs8)*$&lZjA#o49e2_c*@C!FH!+XWMo
z?FP*x=s|7Jk89ni>2_=FC~9~8S|{rHwf0UNH3QGBxuF+FfmiDW4Yv`r-Jn~e;{BTE
zCaw@|#J!-^vi)I@fH&y%_F&hIx6`h9knHXFBPFln-6(8ZL70dz*YkW2uDo!~T3dR;
zI_Es@ELwIi8HE0YwjcU&&_vaZo1F{8m1TSRtYx<?yXE%x!e%Fq!f4R92jOc$=-G+e
z==yzV4Z`g-DM_+daAj?6)q3P<zY(}$TE<>EoeE+mv6*GTxOo+f;gqp(Y`k;_=l_1h
z=_3+b^r_0!yM?vMv2}DnjBHPm5$Q1dE2%lx&ZYwlOXGR|<KF`Y1aL|7V8pZSu+MIy
zC67$LF?5@m9%>!NEaEul;L7EtSk3HED<Fy{OzsC*GXYO)4w-P^HiLW7Z(13bJZBc-
zGs^skH9_nQ`%6E`m)T$XNq(7eszW64{NqSxV6^X2Nj(?pZsfz5rp;#hDC*fAin1Ir
z;WWFS;zu-{GLZN#>-T85T&a1+X~(F$<Fw*0=Yk+d!Kv;Cte<C2dDPWgXI%E@J=UMo
z@GaJ_Gp9VqStjPPEb|1c-_&qc;Vl*@4~<K%(?^WUJTI~SwTA!6`ghD_9=XmxX#T&k
z{*&g<YPNP%S3IL^m^#FN^$^@*oW`WCS6#zpKW}KboCk3Fdp*1rktcD=J5oygVFC_a
z>fki`eQ;vG>pGs_7__&8uoa<A$Zn$%`@8$hZcJ~`{*Z1E`Z5GdfYXe6Js+=lCyWx`
z!Ry`bCvNjqPULsCTd~{ox#RznEnqyYoqBdCWt|$NlPSwu@>PlKgy}b=t2!yN9)AjL
zbbj>s4#&v#mnqb<M%WU5dQQS+{O_bFr2EpJ2K}4W6(15Uo{;Z3fcimxvOfOE2)cl+
zo|6n$8T>+cjT+teMKtLcdH==W;wV%|x-Z=mdHw&DFEANT{U=E8zlWHf6Crq*56?C!
z3hBON&#OPjh2%U@eYy|z`a6hGPT5Zs1---d^#@$mm;80~&Wpdr8|nn-lT5}*ev9L!
zUGmTL4ao)>C;dJ~fiUi_gyAcWA7)4@$ni^<-iN&TTV%%#cBJ{E*ImZTdCjX$Ar>V?
o`i479?;gE7dOWR3-cWz!OL!CQD(T~=ym|3MF7&w?jItj8AJ|S9SpWb4

literal 0
HcmV?d00001

diff --git a/tests/data/test-abidiff-exit/test-net-change.abignore b/tests/data/test-abidiff-exit/test-net-change.abignore
new file mode 100644
index 0000000..6ba2118
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-net-change.abignore
@@ -0,0 +1,8 @@
+[suppress_function]
+  name_regexp = ^fun_
+
+[suppress_variable]
+  name_regexp = ^var_
+
+[suppress_type]
+  name_regexp = ^type_
diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc
index 244de2a..936d1d6 100644
--- a/tests/test-abidiff-exit.cc
+++ b/tests/test-abidiff-exit.cc
@@ -239,6 +239,44 @@ InOutSpec in_out_specs[] =
     "data/test-abidiff-exit/test-decl-enum-report-3.txt",
     "output/test-abidiff-exit/test-decl-enum-report-3.txt"
   },
+  {
+    "data/test-abidiff-exit/test-net-change-v0.o",
+    "data/test-abidiff-exit/test-net-change-v1.o",
+    "",
+    "--no-default-suppression --no-show-locs",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE
+    | abigail::tools_utils::ABIDIFF_ABI_INCOMPATIBLE_CHANGE,
+    "data/test-abidiff-exit/test-net-change-report0.txt",
+    "output/test-abidiff-exit/test-net-change-report0.txt"
+  },
+  {
+    "data/test-abidiff-exit/test-net-change-v0.o",
+    "data/test-abidiff-exit/test-net-change-v1.o",
+    "data/test-abidiff-exit/test-net-change.abignore",
+    "--no-default-suppression --no-show-locs",
+    abigail::tools_utils::ABIDIFF_OK,
+    "data/test-abidiff-exit/test-net-change-report1.txt",
+    "output/test-abidiff-exit/test-net-change-report1.txt"
+  },
+  {
+    "data/test-abidiff-exit/test-net-change-v0.o",
+    "data/test-abidiff-exit/test-net-change-v1.o",
+    "",
+    "--no-default-suppression --no-show-locs --leaf-changes-only",
+    abigail::tools_utils::ABIDIFF_ABI_CHANGE
+    | abigail::tools_utils::ABIDIFF_ABI_INCOMPATIBLE_CHANGE,
+    "data/test-abidiff-exit/test-net-change-report2.txt",
+    "output/test-abidiff-exit/test-net-change-report2.txt"
+  },
+  {
+    "data/test-abidiff-exit/test-net-change-v0.o",
+    "data/test-abidiff-exit/test-net-change-v1.o",
+    "data/test-abidiff-exit/test-net-change.abignore",
+    "--no-default-suppression --no-show-locs --leaf-changes-only",
+    abigail::tools_utils::ABIDIFF_OK,
+    "data/test-abidiff-exit/test-net-change-report3.txt",
+    "output/test-abidiff-exit/test-net-change-report3.txt"
+  },
   {0, 0, 0 ,0,  abigail::tools_utils::ABIDIFF_OK, 0, 0}
 };
  

Hi Dodji

This looks good to me and I concur it's more in keeping with the rest
of the code.

A couple of nits inline.


On Fri, 17 Jul 2020 at 14:54, Dodji Seketeli <dodji@seketeli.org> wrote:
>
> Hello Giuliano,
>
> Giuliano Procida <gprocida@google.com> a écrit:
>
> [...]
>
> > diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
> > index c1d86a7f..e4b86426 100644
> > --- a/src/abg-comparison.cc
> > +++ b/src/abg-comparison.cc
> > @@ -10847,21 +10847,27 @@ corpus_diff::has_net_changes() const
> >      const diff_stats& stats = const_cast<corpus_diff*>(this)->
> >        apply_filters_and_suppressions_before_reporting();
> >
> > +    // Logic here should match emit_diff_stats.
> > +    // TODO: Possibly suppress things that won't be shown there.
> > +    bool leaf = context()->show_leaf_changes_only();
>
> I think that right here, we shouldn't be making assumptions about the
> kind of reporter (leaf or default) we are using.  This is a relic from
> the ancient time predating the support of multiple reporters.  I (maybe
> too) progressively started to delegate reporter-specific behaviour to
> the reporters, especially with how corpus_diff::has_net_changes is
> implemented.
>
> I think we should move further into that direction, to keep a more
> maintainable code base.  In practise, that means that this comparison
> engine should remain as uncoupled from the reporters as possible .
>
> So I have amended your patch a little bit to achieve that, in the same
> spirit as what has been done for corpus_diff::has_net_changes.
>
> Please find below the amended patch I am proposing to apply instead.
> The tests are all the same and pass distcheck.
>
> From 76e53729f426e620268ebb43746476684112023d Mon Sep 17 00:00:00 2001
> From: Giuliano Procida <gprocida@google.com>
> Date: Fri, 17 Jul 2020 14:01:01 +0200
> Subject: [PATCH] Fix corpus_diff::has_net_changes for --leaf-changes-only mode
>
> This function was not aware of --leaf-changes-only mode.
>
>     - Stats counters for changed variables and types have
>       different names in the different modes.
>     - Net leaf type changes were not included in leaf mode.
>
> For some inputs, this resulted in abidiff producing an empty report
> but returning a non-zero exit status in --leaf-changes-only mode.
>
> For other inputs the combination of both issues still resulted in the
> correct return code. This included the following test-abidiff-exit
> test cases:
>
>     - test-leaf-peeling
>     - test-leaf2
>     - test-no-stray-comma
>
> This patch makes corpus_diff::has_net_changes mirror emit_diff_stats,
> modulo flags like --non-reachable-types which if absent can still
> result in discrepancies between output and return code.
>
> To achieve this in a more maintainable way, the patch introduces a new interface
> reporter_base::diff_has_net_changes.  That interface is implemented by
> all current reporters.  Each reporter focuses on its own
> particularities to provide the required behavious. Then
> corpus_diff:has_net_changes just has to invoke
> reporter_base::diff_has_net_changes on the reporter that is currently
> in used.
>
> The tests below verify that the exit code is zero when all the changes
> between the test files are suppressed.
>
>         * include/abg-reporter.h ({reporter_base, default_reporter,
>         leaf_reporter}::diff_has_net_changes): Add new virtual function.
>         This breaks binary compatibility but should conserve source
>         compatibility.
>         * src/abg-default-reporter.cc
>         (default_reporter::diff_has_net_changes): Define new member
>         function.
>         * src/abg-leaf-reporter.cc (leaf_reporter::diff_has_net_changes):
>         Likewise.
>         * src/abg-comparison.cc (corpus_diff::has_net_changes): Invoke
>         reporter_base::diff_has_net_changes on the current reporter,
>         rather than trying to handle all the different kinds of reporters
>         here.
>         (corpus_diff::priv::apply_filters_and_compute_diff_stats): Add a
>         TODO to possibly delegate the implementation of this function to
>         the reporters.
>         * tests/data/Makefile.am: Add new test case files.
>         * tests/data/test-abidiff-exit/test-net-change-report0.txt:
>         Normal mode, nothing suppressed.
>         * tests/data/test-abidiff-exit/test-net-change-report1.txt:
>         Normal mode, everything suppressed.
>         * tests/data/test-abidiff-exit/test-net-change-report2.txt:
>         Leaf mode, nothing suppressed.
>         * tests/data/test-abidiff-exit/test-net-change-report3.txt:
>         Leaf mode, everything suppressions.
>         * tests/data/test-abidiff-exit/test-net-change-v0.c: Test file
>         * tests/data/test-abidiff-exit/test-net-change-v0.o: Test file
>         * tests/data/test-abidiff-exit/test-net-change-v1.c: Test file
>         * tests/data/test-abidiff-exit/test-net-change-v1.o: Test file
>         * tests/data/test-abidiff-exit/test-net-change.abignore: This
>         suppresses changes for all variables, functions and types in
>         the test files, except for the 'victim' function.
>         * tests/test-abidiff-exit.cc: Run new test cases.
>
> Reviewed-by: Matthias Maennich <maennich@google.com>
> Signed-off-by: Giuliano Procida <gprocida@google.com>
> Signed-off-by: Dodji Seketeli <dodji@redhat.com>
> ---
>  include/abg-reporter.h                             |   6 +++
>  src/abg-comparison.cc                              |  24 ++----------
>  src/abg-default-reporter.cc                        |  35 +++++++++++++++++
>  src/abg-leaf-reporter.cc                           |  36 +++++++++++++++++
>  tests/data/Makefile.am                             |   9 +++++
>  .../test-abidiff-exit/test-net-change-report0.txt  |  43 +++++++++++++++++++++
>  .../test-abidiff-exit/test-net-change-report1.txt  |   3 ++
>  .../test-abidiff-exit/test-net-change-report2.txt  |  42 ++++++++++++++++++++
>  .../test-abidiff-exit/test-net-change-report3.txt  |   5 +++
>  tests/data/test-abidiff-exit/test-net-change-v0.c  |  14 +++++++
>  tests/data/test-abidiff-exit/test-net-change-v0.o  | Bin 0 -> 3512 bytes
>  tests/data/test-abidiff-exit/test-net-change-v1.c  |  14 +++++++
>  tests/data/test-abidiff-exit/test-net-change-v1.o  | Bin 0 -> 3552 bytes
>  .../test-abidiff-exit/test-net-change.abignore     |   8 ++++
>  tests/test-abidiff-exit.cc                         |  38 ++++++++++++++++++
>  15 files changed, 257 insertions(+), 20 deletions(-)
>  create mode 100644 tests/data/test-abidiff-exit/test-net-change-report0.txt
>  create mode 100644 tests/data/test-abidiff-exit/test-net-change-report1.txt
>  create mode 100644 tests/data/test-abidiff-exit/test-net-change-report2.txt
>  create mode 100644 tests/data/test-abidiff-exit/test-net-change-report3.txt
>  create mode 100644 tests/data/test-abidiff-exit/test-net-change-v0.c
>  create mode 100644 tests/data/test-abidiff-exit/test-net-change-v0.o
>  create mode 100644 tests/data/test-abidiff-exit/test-net-change-v1.c
>  create mode 100644 tests/data/test-abidiff-exit/test-net-change-v1.o
>  create mode 100644 tests/data/test-abidiff-exit/test-net-change.abignore
>
> diff --git a/include/abg-reporter.h b/include/abg-reporter.h
> index e0d9e66..bf113f0 100644
> --- a/include/abg-reporter.h
> +++ b/include/abg-reporter.h
> @@ -73,6 +73,8 @@ public:
>
>    virtual bool diff_to_be_reported(const diff *d) const;
>
> +  virtual bool diff_has_net_changes(const corpus_diff *d) const = 0;
> +
>    virtual void
>    report(const type_decl_diff& d, std::ostream& out,
>          const std::string& indent = "") const = 0;
> @@ -162,6 +164,8 @@ class default_reporter : public reporter_base
>  {
>  public:
>
> +  virtual bool diff_has_net_changes(const corpus_diff *d) const;
> +
>    virtual void
>    report(const type_decl_diff& d, std::ostream& out,
>          const std::string& indent = "") const;
> @@ -266,6 +270,8 @@ public:
>
>    virtual bool diff_to_be_reported(const diff *d) const;
>
> +  virtual bool diff_has_net_changes(const corpus_diff *d) const;
> +
>    void
>    report_changes_from_diff_maps(const diff_maps&, std::ostream& out,
>                                 const std::string& indent) const;
> diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
> index b6f065f..a9ef161 100644
> --- a/src/abg-comparison.cc
> +++ b/src/abg-comparison.cc
> @@ -10063,6 +10063,9 @@ corpus_diff::priv::apply_filters_and_compute_diff_stats(diff_stats& stat)
>  /// Emit the summary of the functions & variables that got
>  /// removed/changed/added.
>  ///
> +/// TODO: This should be handled by the reporters, just like what is
> +/// done for reporter_base::diff_to_be_reported.
> +///
>  /// @param out the output stream to emit the stats to.
>  ///
>  /// @param indent the indentation string to use in the summary.
> @@ -10848,26 +10851,7 @@ corpus_diff::has_net_subtype_changes() const
>  /// carries subtype changes that are deemed incompatible ABI changes.
>  bool
>  corpus_diff::has_net_changes() const
> -{
> -    const diff_stats& stats = const_cast<corpus_diff*>(this)->
> -      apply_filters_and_suppressions_before_reporting();
> -
> -    return (architecture_changed()
> -           || soname_changed()
> -           || stats.net_num_func_changed()
> -           || stats.net_num_vars_changed()
> -           || stats.net_num_func_added()
> -           || stats.net_num_added_func_syms()
> -           || stats.net_num_func_removed()
> -           || stats.net_num_removed_func_syms()
> -           || stats.net_num_vars_added()
> -           || stats.net_num_added_var_syms()
> -           || stats.net_num_vars_removed()
> -           || stats.net_num_removed_var_syms()
> -           || stats.net_num_added_unreachable_types()
> -           || stats.net_num_removed_unreachable_types()
> -           || stats.net_num_changed_unreachable_types());
> -}
> +{return  context()->get_reporter()->diff_has_net_changes(this);}
>
>  /// Apply the different filters that are registered to be applied to
>  /// the diff tree; that includes the categorization filters.  Also,
> diff --git a/src/abg-default-reporter.cc b/src/abg-default-reporter.cc
> index 2948e15..24a05fa 100644
> --- a/src/abg-default-reporter.cc
> +++ b/src/abg-default-reporter.cc
> @@ -35,6 +35,41 @@ namespace abigail
>  namespace comparison
>  {
>
> +/// Test if a given instance of @ref corpus_diff carries changes whose
> +/// reports are not suppressed by any suppression specification.  In
> +/// effect, these are deemed incompatible ABI changes.
> +///
> +/// @param d the @ref corpus_diff to consider
> +///
> +/// @return true iff @p d carries subtype changes that are deemed
> +/// incompatible ABI changes.
> +bool
> +default_reporter::diff_has_net_changes(const corpus_diff *d) const
> +{
> +  if (!d)
> +    return false;
> +
> +  const corpus_diff::diff_stats& stats = const_cast<corpus_diff*>(d)->
> +    apply_filters_and_suppressions_before_reporting();
> +
> +  // Logic here should match emit_diff_stats.
> +  return (d->architecture_changed()

I have a preference for not doing return (expression). It's not an
issue if you find the extra parentheses help.

> +         ||d->soname_changed()

Missing a space before the "d".

> +         || stats.net_num_func_removed()
> +         || stats.net_num_func_changed()
> +         || stats.net_num_func_added()
> +         || stats.net_num_vars_removed()
> +         || stats.net_num_vars_changed()
> +         || stats.net_num_vars_added()
> +         || stats.net_num_removed_unreachable_types()
> +         || stats.net_num_changed_unreachable_types()
> +         || stats.net_num_added_unreachable_types()
> +         || stats.net_num_removed_func_syms()
> +         || stats.net_num_added_func_syms()
> +         || stats.net_num_removed_var_syms()
> +         || stats.net_num_added_var_syms());
> +}
> +
>  /// Ouputs a report of the differences between of the two type_decl
>  /// involved in the @ref type_decl_diff.
>  ///
> diff --git a/src/abg-leaf-reporter.cc b/src/abg-leaf-reporter.cc
> index 783ffbc..0875c96 100644
> --- a/src/abg-leaf-reporter.cc
> +++ b/src/abg-leaf-reporter.cc
> @@ -45,6 +45,42 @@ bool
>  leaf_reporter::diff_to_be_reported(const diff *d) const
>  {return d && d->to_be_reported() && d->has_local_changes();}
>
> +/// Test if a given instance of @ref corpus_diff carries changes whose
> +/// reports are not suppressed by any suppression specification.  In
> +/// effect, these are deemed incompatible ABI changes.
> +///
> +/// @param d the @ref corpus_diff to consider
> +///
> +/// @return true iff @p d carries subtype changes that are deemed
> +/// incompatible ABI changes.
> +bool
> +leaf_reporter::diff_has_net_changes(const corpus_diff *d) const
> +{
> +  if (!d)
> +    return false;
> +
> +  const corpus_diff::diff_stats& stats = const_cast<corpus_diff*>(d)->
> +    apply_filters_and_suppressions_before_reporting();
> +
> +  // Logic here should match emit_diff_stats.
> +  return (d->architecture_changed()

Could also make this plain return expression.

> +         || d->soname_changed()
> +         || stats.net_num_func_removed()
> +         || stats.net_num_leaf_type_changes()
> +         || stats.net_num_leaf_func_changes()
> +         || stats.net_num_func_added()
> +         || stats.net_num_vars_removed()
> +         || stats.net_num_leaf_var_changes()
> +         || stats.net_num_vars_added()
> +         || stats.net_num_removed_unreachable_types()
> +         || stats.net_num_changed_unreachable_types()
> +         || stats.net_num_added_unreachable_types()
> +         || stats.net_num_removed_func_syms()
> +         || stats.net_num_added_func_syms()
> +         || stats.net_num_removed_var_syms()
> +         || stats.net_num_added_var_syms());
> +}
> +
>  /// Report the changes carried by the diffs contained in an instance
>  /// of @ref string_diff_ptr_map.
>  ///
> diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
> index 5da9ef9..ea94f0b 100644
> --- a/tests/data/Makefile.am
> +++ b/tests/data/Makefile.am
> @@ -165,6 +165,15 @@ test-abidiff-exit/test-decl-enum-v1.o \
>  test-abidiff-exit/test-decl-enum-report.txt \
>  test-abidiff-exit/test-decl-enum-report-2.txt \
>  test-abidiff-exit/test-decl-enum-report-3.txt \
> +test-abidiff-exit/test-net-change.abignore \
> +test-abidiff-exit/test-net-change-v0.c \
> +test-abidiff-exit/test-net-change-v0.o \
> +test-abidiff-exit/test-net-change-v1.c \
> +test-abidiff-exit/test-net-change-v1.o \
> +test-abidiff-exit/test-net-change-report0.txt \
> +test-abidiff-exit/test-net-change-report1.txt \
> +test-abidiff-exit/test-net-change-report2.txt \
> +test-abidiff-exit/test-net-change-report3.txt \
>  \
>  test-diff-dwarf/test0-v0.cc            \
>  test-diff-dwarf/test0-v0.o                     \
> diff --git a/tests/data/test-abidiff-exit/test-net-change-report0.txt b/tests/data/test-abidiff-exit/test-net-change-report0.txt
> new file mode 100644
> index 0000000..66712b0
> --- /dev/null
> +++ b/tests/data/test-abidiff-exit/test-net-change-report0.txt
> @@ -0,0 +1,43 @@
> +Functions changes summary: 1 Removed, 2 Changed, 1 Added functions
> +Variables changes summary: 1 Removed, 1 Changed, 1 Added variables
> +
> +1 Removed function:
> +
> +  [D] 'function int fun_removed()'    {fun_removed}
> +
> +1 Added function:
> +
> +  [A] 'function long int fun_added()'    {fun_added}
> +
> +2 functions with some indirect sub-type change:
> +
> +  [C] 'function int fun_changed()' has some indirect sub-type changes:
> +    return type changed:
> +      type name changed from 'int' to 'long int'
> +      type size changed from 32 to 64 (in bits)
> +
> +  [C] 'function void victim(type_changed*)' has some indirect sub-type changes:
> +    parameter 1 of type 'type_changed*' has sub-type changes:
> +      in pointed to type 'struct type_changed':
> +        type size changed from 32 to 64 (in bits)
> +        1 data member change:
> +          type of 'int type_changed::x' changed:
> +            type name changed from 'int' to 'long int'
> +            type size changed from 32 to 64 (in bits)
> +
> +1 Removed variable:
> +
> +  [D] 'int var_removed'    {var_removed}
> +
> +1 Added variable:
> +
> +  [A] 'long int var_added'    {var_added}
> +
> +1 Changed variable:
> +
> +  [C] 'int var_changed' was changed to 'long int var_changed':
> +    size of symbol changed from 4 to 8
> +    type of variable changed:
> +      type name changed from 'int' to 'long int'
> +      type size changed from 32 to 64 (in bits)
> +
> diff --git a/tests/data/test-abidiff-exit/test-net-change-report1.txt b/tests/data/test-abidiff-exit/test-net-change-report1.txt
> new file mode 100644
> index 0000000..4dd6096
> --- /dev/null
> +++ b/tests/data/test-abidiff-exit/test-net-change-report1.txt
> @@ -0,0 +1,3 @@
> +Functions changes summary: 0 Removed (1 filtered out), 0 Changed (2 filtered out), 0 Added (1 filtered out) functions
> +Variables changes summary: 0 Removed (1 filtered out), 0 Changed (1 filtered out), 0 Added (1 filtered out) variables
> +
> diff --git a/tests/data/test-abidiff-exit/test-net-change-report2.txt b/tests/data/test-abidiff-exit/test-net-change-report2.txt
> new file mode 100644
> index 0000000..ca3b3e0
> --- /dev/null
> +++ b/tests/data/test-abidiff-exit/test-net-change-report2.txt
> @@ -0,0 +1,42 @@
> +Leaf changes summary: 7 artifacts changed
> +Changed leaf types summary: 1 leaf type changed
> +Removed/Changed/Added functions summary: 1 Removed, 1 Changed, 1 Added function
> +Removed/Changed/Added variables summary: 1 Removed, 1 Changed, 1 Added variable
> +
> +1 Removed function:
> +
> +  [D] 'function int fun_removed()'    {fun_removed}
> +
> +1 Added function:
> +
> +  [A] 'function long int fun_added()'    {fun_added}
> +
> +1 function with some sub-type change:
> +
> +  [C] 'function int fun_changed()' has some sub-type changes:
> +    return type changed:
> +      type name changed from 'int' to 'long int'
> +      type size changed from 32 to 64 (in bits)
> +
> +1 Removed variable:
> +
> +  [D] 'int var_removed'    {var_removed}
> +
> +1 Added variable:
> +
> +  [A] 'long int var_added'    {var_added}
> +
> +1 Changed variable:
> +
> +  [C] 'int var_changed' was changed to 'long int var_changed':
> +    size of symbol changed from 4 to 8
> +    type of variable changed:
> +      type name changed from 'int' to 'long int'
> +      type size changed from 32 to 64 (in bits)
> +
> +'struct type_changed' changed:
> +  type size changed from 32 to 64 (in bits)
> +  there are data member changes:
> +    type 'int' of 'type_changed::x' changed:
> +      type name changed from 'int' to 'long int'
> +      type size changed from 32 to 64 (in bits)
> diff --git a/tests/data/test-abidiff-exit/test-net-change-report3.txt b/tests/data/test-abidiff-exit/test-net-change-report3.txt
> new file mode 100644
> index 0000000..4856a77
> --- /dev/null
> +++ b/tests/data/test-abidiff-exit/test-net-change-report3.txt
> @@ -0,0 +1,5 @@
> +Leaf changes summary: 0 artifact changed (3 filtered out)
> +Changed leaf types summary: 0 (1 filtered out) leaf type changed
> +Removed/Changed/Added functions summary: 0 Removed (1 filtered out), 0 Changed (1 filtered out), 0 Added function (1 filtered out)
> +Removed/Changed/Added variables summary: 0 Removed (1 filtered out), 0 Changed (1 filtered out), 0 Added variable (1 filtered out)
> +
> diff --git a/tests/data/test-abidiff-exit/test-net-change-v0.c b/tests/data/test-abidiff-exit/test-net-change-v0.c
> new file mode 100644
> index 0000000..684b9c6
> --- /dev/null
> +++ b/tests/data/test-abidiff-exit/test-net-change-v0.c
> @@ -0,0 +1,14 @@
> +int var_removed = 0;
> +int fun_removed() { return 0; }
> +
> +int var_changed = 0;
> +int fun_changed() { return 0; }
> +
> +struct type_removed {
> +  int x;
> +};
> +struct type_changed {
> +  int x;
> +};
> +
> +void victim(struct type_changed * dummy) { (void)dummy->x; }
> diff --git a/tests/data/test-abidiff-exit/test-net-change-v0.o b/tests/data/test-abidiff-exit/test-net-change-v0.o
> new file mode 100644
> index 0000000000000000000000000000000000000000..bd2c744744880e83ba295ba16995c32a7438ec53
> GIT binary patch
> literal 3512
> zcmbtW&2Jk;6o2cD6Q^!mCoa$^MRE(M6uRp~R3%MGYY3zXA611^sS;G_+B>#a*t^ks
> z9h_DOwL(a}6$z<D;#MSXh$FpK>Xkcx0US6W;8ei@-kY5lPbM32;7OkM=J$T@&CHvR
> z^`~#WyPC5sV6osljCza$JSmLqx-{!hff=~6cIW3GsDJITVXocz<jLJ@kJ0rD{>F#{
> z1)2Jww6*4#X6MMZto#b9Nt+$^SUw07NS$MbQ|2DC4ivVLo*#l$oS<lcacZoRRAd&Z
> zL9xF67TXDFmYyR!_eBQ^h3jZdN;EHrAd_SimwI#ic)^;sUMN+Hc4@9OZOu%SpTW$h
> z!7eQo&lS%W*DDuhb4LK|=b>0UU$n8LV-?FT6^p6gs^Ijez%DR-5eFW3tqZf9;lwPZ
> ztslAf#w?L1@>*`nTsVZ8*U>ZS!YFFJvJ-`iIfXx2N6@wuF?|!TyCWZ<Wy8zZTGrpC
> zDQx9&!JV`q5&gsoMdCC&Uf34S*1X%4f%ytdrh{O&l0W)(-nyEvEGXk8d(zDxzMZo%
> zS!)n(#G)5%2_LqCW)k$EHt5H-Zq)R;wRRM>yQ0>KdZO0ejH71Ydo?fg<0$ZJ-Jsz$
> zg0>fQYc!F5&G!;dnhxTA&}uni7$o2idcAGf^5Ttjnm#1kn_|?-S_Dmt*%vQesje)%
> zT0QH&;?7r{UNQ*9a$AHV4w~5Y;$~-gxOm1nbGquZt4_=7Z->oJ9EH)K?+n6^gV1*p
> zuhA8KISF1ZTwGaMs=oN9XaruECU+K3rUo3AWyBiacpt1`*_t~zQMijWKZ-d0NaV46
> zy;S}@w=y-piUh>S_Zm4-_Va(3x-<RCwMXI5c!&S^XO9B`+{P>z@tq^E%WtD6OKkN@
> zm_9qb$R><g<Q2}q<u%Q~WkoXZsXZ70Hk9_k_q<^6G%}~Pb*1kez;VQ9wEFN2T*YS|
> z>m|i!9_v+w(>NrIXJ0`&>wL_1T`=tq=qFrVJj}~)<pIs6@p#3AQ*|E@h%%ijVEne~
> zKV{(fCP|r9nof*<I!-E_^62jp)ro}j|CV;)oYIs>-@SJf&ip|2uWI~5$|mJ0oB0FP
> zzhU6K{#y#?`SX6<H~8r@2=JYO-&6gEMjqbJ9}T|h!cPXDaecq?45BXY!>98_!t+mi
> z099qsp6L5hH*lWkbpz)*fZN~h;bVt7iBsKSD-pv4TzaIzZS?!##-i)FzGw{E8$sBL
> z&?jxL(TK&?F1Huc_holTHwXoX;1J+8qh3$ozXUgo65-;j?er6``H@N_IvcIn>j^dS
> z|EcCMo@&VT=X7;t9AiLf;_5<v!pxV@(@C0b#-BhR-4iq3S26k>B8cA?<%R_`%$(%o
> z_+Oc*w8=e%IOVJ>KdZ7T$oDFM&LQ!5KKLg^$}+ZQPI6pQ;5U*rYE15BbeR~j|5V`o
> zC{$V|m)?o2`9D&h6pp9)6J*ZcN6gHL5FA&RXM;MLm|XH_jsLqEpX)^9(|c=<zlj*-
> z<aweg<(3-%F9ww6@mb$QJS+aDI#AF4NBn0h-k`yK^FS&BkIVe8RJ{JykdNbsOjMfJ
> z&+wOMWYw>rh-->S^(*0z<GHRhA9HL9DN|AuKEL#Rqwk9uPkWNJ)JN(Oeh>Y+nCqv!
> QS@D<EKo9g_w9WW`0Y2;*ng9R*
>
> literal 0
> HcmV?d00001
>
> diff --git a/tests/data/test-abidiff-exit/test-net-change-v1.c b/tests/data/test-abidiff-exit/test-net-change-v1.c
> new file mode 100644
> index 0000000..7b2a4ad
> --- /dev/null
> +++ b/tests/data/test-abidiff-exit/test-net-change-v1.c
> @@ -0,0 +1,14 @@
> +long var_added = 0;
> +long fun_added() { return 0; }
> +
> +long var_changed = 0;
> +long fun_changed() { return 0; }
> +
> +struct type_added {
> +  long x;
> +};
> +struct type_changed {
> +  long x;
> +};
> +
> +void victim(struct type_changed * dummy) { (void)dummy->x; }
> diff --git a/tests/data/test-abidiff-exit/test-net-change-v1.o b/tests/data/test-abidiff-exit/test-net-change-v1.o
> new file mode 100644
> index 0000000000000000000000000000000000000000..994ca58ecc5c35e7ecc51da87b685379c2734d6d
> GIT binary patch
> literal 3552
> zcmbtW-D@0G6hE`mO*Y+Tn`EOV#KIImYO6DwV2NpJ+qKa&Q4uK=3mVedo!Q;N-I*{u
> z8#fgZr3%HD3W5qEh=M+ekNVsfAAIo5zd%vnL=eFTJ?GB3vo|-BClAa$=lsss{l4?&
> zGtaLV3<D$?a1lnDL;<Xckz5yI9jY(`*EVkd{4MEQcQtY2_8WiS*}5xy0e(S#M93wP
> z{utvKJDEuU1(FS8Yz^6jDGg?+NMdR{q8UZ*l5w=Ohc3k-808X01B_8)R0JcjObwFp
> z$(PuU3$gM5$%XejP%7O(NsQGfiW1~NG>S{Dg+r`hOdAhXs%5jXP?<Jn#;5MX%xA%@
> ztd`H0&y_c;m*xv|0Oo^GE?-11oR+F#R?6knZd9@RlVFyl`wA8wbBs&#GQ+8PN?V`%
> z@bWx4j~A8Pl(}>qGjF10!huoLW;Kh#!90eam`5nl*fLTVAp=o(89x&q#nv$XsZ3%k
> zwhXSM0g2yF?9fl_X2%WNzP-ETG(~K21}4%Xs8)*$&lZjA#o49e2_c*@C!FH!+XWMo
> z?FP*x=s|7Jk89ni>2_=FC~9~8S|{rHwf0UNH3QGBxuF+FfmiDW4Yv`r-Jn~e;{BTE
> zCaw@|#J!-^vi)I@fH&y%_F&hIx6`h9knHXFBPFln-6(8ZL70dz*YkW2uDo!~T3dR;
> zI_Es@ELwIi8HE0YwjcU&&_vaZo1F{8m1TSRtYx<?yXE%x!e%Fq!f4R92jOc$=-G+e
> z==yzV4Z`g-DM_+daAj?6)q3P<zY(}$TE<>EoeE+mv6*GTxOo+f;gqp(Y`k;_=l_1h
> z=_3+b^r_0!yM?vMv2}DnjBHPm5$Q1dE2%lx&ZYwlOXGR|<KF`Y1aL|7V8pZSu+MIy
> zC67$LF?5@m9%>!NEaEul;L7EtSk3HED<Fy{OzsC*GXYO)4w-P^HiLW7Z(13bJZBc-
> zGs^skH9_nQ`%6E`m)T$XNq(7eszW64{NqSxV6^X2Nj(?pZsfz5rp;#hDC*fAin1Ir
> z;WWFS;zu-{GLZN#>-T85T&a1+X~(F$<Fw*0=Yk+d!Kv;Cte<C2dDPWgXI%E@J=UMo
> z@GaJ_Gp9VqStjPPEb|1c-_&qc;Vl*@4~<K%(?^WUJTI~SwTA!6`ghD_9=XmxX#T&k
> z{*&g<YPNP%S3IL^m^#FN^$^@*oW`WCS6#zpKW}KboCk3Fdp*1rktcD=J5oygVFC_a
> z>fki`eQ;vG>pGs_7__&8uoa<A$Zn$%`@8$hZcJ~`{*Z1E`Z5GdfYXe6Js+=lCyWx`
> z!Ry`bCvNjqPULsCTd~{ox#RznEnqyYoqBdCWt|$NlPSwu@>PlKgy}b=t2!yN9)AjL
> zbbj>s4#&v#mnqb<M%WU5dQQS+{O_bFr2EpJ2K}4W6(15Uo{;Z3fcimxvOfOE2)cl+
> zo|6n$8T>+cjT+teMKtLcdH==W;wV%|x-Z=mdHw&DFEANT{U=E8zlWHf6Crq*56?C!
> z3hBON&#OPjh2%U@eYy|z`a6hGPT5Zs1---d^#@$mm;80~&Wpdr8|nn-lT5}*ev9L!
> zUGmTL4ao)>C;dJ~fiUi_gyAcWA7)4@$ni^<-iN&TTV%%#cBJ{E*ImZTdCjX$Ar>V?
> o`i479?;gE7dOWR3-cWz!OL!CQD(T~=ym|3MF7&w?jItj8AJ|S9SpWb4
>
> literal 0
> HcmV?d00001
>
> diff --git a/tests/data/test-abidiff-exit/test-net-change.abignore b/tests/data/test-abidiff-exit/test-net-change.abignore
> new file mode 100644
> index 0000000..6ba2118
> --- /dev/null
> +++ b/tests/data/test-abidiff-exit/test-net-change.abignore
> @@ -0,0 +1,8 @@
> +[suppress_function]
> +  name_regexp = ^fun_
> +
> +[suppress_variable]
> +  name_regexp = ^var_
> +
> +[suppress_type]
> +  name_regexp = ^type_
> diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc
> index 244de2a..936d1d6 100644
> --- a/tests/test-abidiff-exit.cc
> +++ b/tests/test-abidiff-exit.cc
> @@ -239,6 +239,44 @@ InOutSpec in_out_specs[] =
>      "data/test-abidiff-exit/test-decl-enum-report-3.txt",
>      "output/test-abidiff-exit/test-decl-enum-report-3.txt"
>    },
> +  {
> +    "data/test-abidiff-exit/test-net-change-v0.o",
> +    "data/test-abidiff-exit/test-net-change-v1.o",
> +    "",
> +    "--no-default-suppression --no-show-locs",
> +    abigail::tools_utils::ABIDIFF_ABI_CHANGE
> +    | abigail::tools_utils::ABIDIFF_ABI_INCOMPATIBLE_CHANGE,
> +    "data/test-abidiff-exit/test-net-change-report0.txt",
> +    "output/test-abidiff-exit/test-net-change-report0.txt"
> +  },
> +  {
> +    "data/test-abidiff-exit/test-net-change-v0.o",
> +    "data/test-abidiff-exit/test-net-change-v1.o",
> +    "data/test-abidiff-exit/test-net-change.abignore",
> +    "--no-default-suppression --no-show-locs",
> +    abigail::tools_utils::ABIDIFF_OK,
> +    "data/test-abidiff-exit/test-net-change-report1.txt",
> +    "output/test-abidiff-exit/test-net-change-report1.txt"
> +  },
> +  {
> +    "data/test-abidiff-exit/test-net-change-v0.o",
> +    "data/test-abidiff-exit/test-net-change-v1.o",
> +    "",
> +    "--no-default-suppression --no-show-locs --leaf-changes-only",
> +    abigail::tools_utils::ABIDIFF_ABI_CHANGE
> +    | abigail::tools_utils::ABIDIFF_ABI_INCOMPATIBLE_CHANGE,
> +    "data/test-abidiff-exit/test-net-change-report2.txt",
> +    "output/test-abidiff-exit/test-net-change-report2.txt"
> +  },
> +  {
> +    "data/test-abidiff-exit/test-net-change-v0.o",
> +    "data/test-abidiff-exit/test-net-change-v1.o",
> +    "data/test-abidiff-exit/test-net-change.abignore",
> +    "--no-default-suppression --no-show-locs --leaf-changes-only",
> +    abigail::tools_utils::ABIDIFF_OK,
> +    "data/test-abidiff-exit/test-net-change-report3.txt",
> +    "output/test-abidiff-exit/test-net-change-report3.txt"
> +  },
>    {0, 0, 0 ,0,  abigail::tools_utils::ABIDIFF_OK, 0, 0}
>  };
>
> --
> 1.8.3.1
>
>
> --
>                 Dodji

Thank you for the review and follow-up!

Regards,
Giuliano.
  

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

[...]

> This looks good to me and I concur it's more in keeping with the rest
> of the code.
>
> A couple of nits inline.

[...]

>> --- a/src/abg-default-reporter.cc
>> +++ b/src/abg-default-reporter.cc

[...]

>> +/// Test if a given instance of @ref corpus_diff carries changes whose
>> +/// reports are not suppressed by any suppression specification.  In
>> +/// effect, these are deemed incompatible ABI changes.
>> +///
>> +/// @param d the @ref corpus_diff to consider
>> +///
>> +/// @return true iff @p d carries subtype changes that are deemed
>> +/// incompatible ABI changes.
>> +bool
>> +default_reporter::diff_has_net_changes(const corpus_diff *d) const
>> +{
>> +  if (!d)
>> +    return false;
>> +
>> +  const corpus_diff::diff_stats& stats = const_cast<corpus_diff*>(d)->
>> +    apply_filters_and_suppressions_before_reporting();
>> +
>> +  // Logic here should match emit_diff_stats.
>> +  return (d->architecture_changed()
>
> I have a preference for not doing return (expression). It's not an
> issue if you find the extra parentheses help.

Ah, I keep the parenthesis around the returned expression there when the
return statements spans over multiple lines.  In that case, the editor
indents the returned expression "on its own", like it does for function
parameters etc.

>
>> +         ||d->soname_changed()
>
> Missing a space before the "d".

Fixed, thanks.

>
>> +         || stats.net_num_func_removed()
>> +         || stats.net_num_func_changed()
>> +         || stats.net_num_func_added()
>> +         || stats.net_num_vars_removed()
>> +         || stats.net_num_vars_changed()
>> +         || stats.net_num_vars_added()
>> +         || stats.net_num_removed_unreachable_types()
>> +         || stats.net_num_changed_unreachable_types()
>> +         || stats.net_num_added_unreachable_types()
>> +         || stats.net_num_removed_func_syms()
>> +         || stats.net_num_added_func_syms()
>> +         || stats.net_num_removed_var_syms()
>> +         || stats.net_num_added_var_syms());
>> +}
>> +

[...]

>> +bool
>> +leaf_reporter::diff_has_net_changes(const corpus_diff *d) const
>> +{
>> +  if (!d)
>> +    return false;
>> +
>> +  const corpus_diff::diff_stats& stats = const_cast<corpus_diff*>(d)->
>> +    apply_filters_and_suppressions_before_reporting();
>> +
>> +  // Logic here should match emit_diff_stats.
>> +  return (d->architecture_changed()
>
> Could also make this plain return expression.

I kept it for proper indentation purpuses as well.

> Thank you for the review and follow-up!

No problem.  Applied to master.

Cheers,