[RFC] Fix has_net_changes for leaf-changes-only mode.

[RFC] Fix has_net_changes for leaf-changes-only mode.

[Giuliano Procida]

* Which other functions need the same treatment?
* Should something else change instead?
* What about tracking of changed types (inclusion of these in summary
  stats and exit code for leaf mode)?

This patch breaks tests and is not suitable for applying in its
current form.

The issue we saw is that anonymous struct name changes triggered
hundreds of diffs (mostly but not all filtered out in non-leaf mode
which may be its own bug).

	* src/abg-comparison.cc (corpus_diff::has_net_changes): In
        leaf-changes-only mode, check leaf changes to functions and
        variables to avoid returning non-zero exit code with empty
        output.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 src/abg-comparison.cc | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index 46bf9e30..7756c12b 100644
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -10602,21 +10602,26 @@ corpus_diff::has_net_changes() const
     const diff_stats& stats = const_cast<corpus_diff*>(this)->
       apply_filters_and_suppressions_before_reporting();
 
+    bool leaf = context()->show_leaf_changes_only();
     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());
+            || soname_changed()
+            || (leaf
+                  ? stats.net_num_leaf_func_changes()
+                  : stats.net_num_func_changed())
+            || (leaf
+                  ? stats.net_num_leaf_var_changes()
+                  : 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());
 }
 
 /// Apply the different filters that are registered to be applied to
-- 
2.25.1.696.g5e7596f4ac-goog

Re: [RFC] Fix has_net_changes for leaf-changes-only mode.

[Giuliano Procida]

Dodji, Matthias.

The test breakage I mentioned is returning exit code 0 despite type
changes impacting functions.

Comments welcome.

Giuliano.

On Tue, 24 Mar 2020 at 17:13, Giuliano Procida <gprocida@google.com> wrote:
>
> * Which other functions need the same treatment?
> * Should something else change instead?
> * What about tracking of changed types (inclusion of these in summary
>   stats and exit code for leaf mode)?
>
> This patch breaks tests and is not suitable for applying in its
> current form.
>
> The issue we saw is that anonymous struct name changes triggered
> hundreds of diffs (mostly but not all filtered out in non-leaf mode
> which may be its own bug).
>
>         * src/abg-comparison.cc (corpus_diff::has_net_changes): In
>         leaf-changes-only mode, check leaf changes to functions and
>         variables to avoid returning non-zero exit code with empty
>         output.
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>
> ---
>  src/abg-comparison.cc | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
> index 46bf9e30..7756c12b 100644
> --- a/src/abg-comparison.cc
> +++ b/src/abg-comparison.cc
> @@ -10602,21 +10602,26 @@ corpus_diff::has_net_changes() const
>      const diff_stats& stats = const_cast<corpus_diff*>(this)->
>        apply_filters_and_suppressions_before_reporting();
>
> +    bool leaf = context()->show_leaf_changes_only();
>      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());
> +            || soname_changed()
> +            || (leaf
> +                  ? stats.net_num_leaf_func_changes()
> +                  : stats.net_num_func_changed())
> +            || (leaf
> +                  ? stats.net_num_leaf_var_changes()
> +                  : 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());
>  }
>
>  /// Apply the different filters that are registered to be applied to
> --
> 2.25.1.696.g5e7596f4ac-goog
>

Re: [RFC] Fix has_net_changes for leaf-changes-only mode.

[Matthias Maennich]

On Tue, Mar 24, 2020 at 05:13:44PM +0000, Giuliano Procida wrote:
>* Which other functions need the same treatment?
>* Should something else change instead?
>* What about tracking of changed types (inclusion of these in summary
>  stats and exit code for leaf mode)?
>
>This patch breaks tests and is not suitable for applying in its
>current form.
>
>The issue we saw is that anonymous struct name changes triggered
>hundreds of diffs (mostly but not all filtered out in non-leaf mode
>which may be its own bug).
>
>	* src/abg-comparison.cc (corpus_diff::has_net_changes): In
>        leaf-changes-only mode, check leaf changes to functions and
>        variables to avoid returning non-zero exit code with empty
>        output.
>

That patch looks generally good to me. Do you have an idea how to
address the test breakages?

>Signed-off-by: Giuliano Procida <gprocida@google.com>
>---
> src/abg-comparison.cc | 33 +++++++++++++++++++--------------
> 1 file changed, 19 insertions(+), 14 deletions(-)
>
>diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
>index 46bf9e30..7756c12b 100644
>--- a/src/abg-comparison.cc
>+++ b/src/abg-comparison.cc
>@@ -10602,21 +10602,26 @@ corpus_diff::has_net_changes() const
>     const diff_stats& stats = const_cast<corpus_diff*>(this)->
>       apply_filters_and_suppressions_before_reporting();
>
>+    bool leaf = context()->show_leaf_changes_only();

const

Cheers,
Matthias

>     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());
>+            || soname_changed()
>+            || (leaf
>+                  ? stats.net_num_leaf_func_changes()
>+                  : stats.net_num_func_changed())
>+            || (leaf
>+                  ? stats.net_num_leaf_var_changes()
>+                  : 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());
> }
>
> /// Apply the different filters that are registered to be applied to
>-- 
>2.25.1.696.g5e7596f4ac-goog
>

Re: [RFC] Fix has_net_changes for leaf-changes-only mode.

[Giuliano Procida]

Hi Matthias.

On Thu, 26 Mar 2020 at 10:37, Matthias Maennich <maennich@google.com> wrote:
>
> On Tue, Mar 24, 2020 at 05:13:44PM +0000, Giuliano Procida wrote:
> >* Which other functions need the same treatment?
> >* Should something else change instead?
> >* What about tracking of changed types (inclusion of these in summary
> >  stats and exit code for leaf mode)?
> >
> >This patch breaks tests and is not suitable for applying in its
> >current form.
> >
> >The issue we saw is that anonymous struct name changes triggered
> >hundreds of diffs (mostly but not all filtered out in non-leaf mode
> >which may be its own bug).
> >
> >       * src/abg-comparison.cc (corpus_diff::has_net_changes): In
> >        leaf-changes-only mode, check leaf changes to functions and
> >        variables to avoid returning non-zero exit code with empty
> >        output.
> >
>
> That patch looks generally good to me. Do you have an idea how to
> address the test breakages?

Here's an example (the same without --impacted-interfaces):

$ build/tools/abidiff --impacted-interfaces --leaf-changes-only
tests/data/test-abidiff-exit/test-leaf2-v?.o
Leaf changes summary: 1 artifact changed
Changed leaf types summary: 1 leaf type changed
Removed/Changed/Added functions summary: 0 Removed, 0 Changed, 0 Added function
Removed/Changed/Added variables summary: 0 Removed, 0 Changed, 0 Added variable

'struct ops at test-leaf2-v0.cc:1:1' changed:
  type size hasn't changed
  there are data member changes:
    type 'void (int)*' of 'ops::munge' changed:
      pointer type changed from: 'void (int)*' to: 'char (long int, bool)*'

  one impacted interface:
    function void register_ops(const ops&)
$ echo $?
0

Exit code 0 is wrong. We should be counting and reporting changed
types and using the count as part of the exit code.

> >Signed-off-by: Giuliano Procida <gprocida@google.com>
> >---
> > src/abg-comparison.cc | 33 +++++++++++++++++++--------------
> > 1 file changed, 19 insertions(+), 14 deletions(-)
> >
> >diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
> >index 46bf9e30..7756c12b 100644
> >--- a/src/abg-comparison.cc
> >+++ b/src/abg-comparison.cc
> >@@ -10602,21 +10602,26 @@ corpus_diff::has_net_changes() const
> >     const diff_stats& stats = const_cast<corpus_diff*>(this)->
> >       apply_filters_and_suppressions_before_reporting();
> >
> >+    bool leaf = context()->show_leaf_changes_only();
>
> const

I think it's OK as-is. There are no instances of local "const bool" in
the code. This bool is only used in the next expression.

> Cheers,
> Matthias

Regards,
Giuliano.

> >     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());
> >+            || soname_changed()
> >+            || (leaf
> >+                  ? stats.net_num_leaf_func_changes()
> >+                  : stats.net_num_func_changed())
> >+            || (leaf
> >+                  ? stats.net_num_leaf_var_changes()
> >+                  : 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());
> > }
> >
> > /// Apply the different filters that are registered to be applied to
> >--
> >2.25.1.696.g5e7596f4ac-goog
> >

Re: [RFC] Fix has_net_changes for leaf-changes-only mode.

[Dodji Seketeli]

Hello Giuliano,

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

> * Which other functions need the same treatment?
> * Should something else change instead?
> * What about tracking of changed types (inclusion of these in summary
>   stats and exit code for leaf mode)?
>
> This patch breaks tests and is not suitable for applying in its
> current form.
>
> The issue we saw is that anonymous struct name changes triggered
> hundreds of diffs

Sorry, but what do you mean by "anonymous struct name changes" ?

[...]

> diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
> index 46bf9e30..7756c12b 100644
> --- a/src/abg-comparison.cc
> +++ b/src/abg-comparison.cc
> @@ -10602,21 +10602,26 @@ corpus_diff::has_net_changes() const
>      const diff_stats& stats = const_cast<corpus_diff*>(this)->
>        apply_filters_and_suppressions_before_reporting();
>  
> +    bool leaf = context()->show_leaf_changes_only();
>      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());
> +            || soname_changed()
> +            || (leaf
> +                  ? stats.net_num_leaf_func_changes()
> +                  : stats.net_num_func_changed())
> +            || (leaf
> +                  ? stats.net_num_leaf_var_changes()
> +                  : stats.net_num_vars_changed())

In essence, this is code which behaviour depends on the kind of
"reporter" we are using.  That is, if we are using the leaf reporter,
then we want a given behaviour, otherwise we want another behaviour.

So I think a route to make the whole thing maintainable going forward
would be to put that code in the reporters.

That is, in include/abg-reporter.h, add a pure virtual
bool reporter_base::diff_has_net_changes(const corpus_diff&) const function.

That interface would then be implemented in both default_reporter and
leaf_reporter class types.

Then in corpus_diff::has_net_changes(), we'd do something like:

  return context()->get_reporter()->diff_has_net_changes(*this);

That way, the actual choice of what constitute the set of net changes
would be left to the each reporter.

Does that make any sense?

[...]


> (mostly but not all filtered out in non-leaf mode which may be its own
> bug).

Hopefully with this approach, this issue should go away as well.

Thanks a lot for looking into this.

Cheers,

-- 
		Dodji

Re: [RFC] Fix has_net_changes for leaf-changes-only mode.

[Giuliano Procida]

Hi Dodji.

On Thu, 26 Mar 2020 at 17:12, Dodji Seketeli <dodji@seketeli.org> wrote:
>
> Hello Giuliano,
>
> Giuliano Procida <gprocida@google.com> a écrit:
>
> > * Which other functions need the same treatment?
> > * Should something else change instead?
> > * What about tracking of changed types (inclusion of these in summary
> >   stats and exit code for leaf mode)?
> >
> > This patch breaks tests and is not suitable for applying in its
> > current form.
> >
> > The issue we saw is that anonymous struct name changes triggered
> > hundreds of diffs
>
> Sorry, but what do you mean by "anonymous struct name changes" ?

These were kernel ABI differences. Freshly produced XML compared
against a recorded XML ABI. The recorded ABI was produced by a
slightly different version (of abidw). 1.8.0-fc4af1c4-android vs
1.8.0-7bd6f34f-android. Those are just libabigail commit hashes.
Current mm-next vs 2 merges ago.

With the --harmless and default reporting mode:1.8.0-7bd6f34f-android

Functions changes summary: 0 Removed, 410 Changed, 0 Added functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

410 functions with some indirect sub-type change:

[...]

          while looking at anonymous data member 'union {struct
{sk_buff* next; sk_buff* prev; union {net_device* dev; unsigned long
int dev_scratch;};}; rb_node rbnode; list_head list;}':
          the internal name of that anonymous data memberchanged from:
           __anonymous_union__
          to:
           __anonymous_union__16
           This is usually due to an anonymous member type being added
or removed from the containing type

[many more]

With --harmless and leaf reporting mode:

Leaf changes summary: 0 artifact changed
Changed leaf types summary: 0 leaf type changed
Removed/Changed/Added functions summary: 0 Removed, 0 Changed, 0 Added function
Removed/Changed/Added variables summary: 0 Removed, 0 Changed, 0 Added variable

(exit status 4)

> [...]
>
> > diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
> > index 46bf9e30..7756c12b 100644
> > --- a/src/abg-comparison.cc
> > +++ b/src/abg-comparison.cc
> > @@ -10602,21 +10602,26 @@ corpus_diff::has_net_changes() const
> >      const diff_stats& stats = const_cast<corpus_diff*>(this)->
> >        apply_filters_and_suppressions_before_reporting();
> >
> > +    bool leaf = context()->show_leaf_changes_only();
> >      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());
> > +            || soname_changed()
> > +            || (leaf
> > +                  ? stats.net_num_leaf_func_changes()
> > +                  : stats.net_num_func_changed())
> > +            || (leaf
> > +                  ? stats.net_num_leaf_var_changes()
> > +                  : stats.net_num_vars_changed())
>
> In essence, this is code which behaviour depends on the kind of
> "reporter" we are using.  That is, if we are using the leaf reporter,
> then we want a given behaviour, otherwise we want another behaviour.
>
> So I think a route to make the whole thing maintainable going forward
> would be to put that code in the reporters.
>
> That is, in include/abg-reporter.h, add a pure virtual
> bool reporter_base::diff_has_net_changes(const corpus_diff&) const function.
>
> That interface would then be implemented in both default_reporter and
> leaf_reporter class types.
>
> Then in corpus_diff::has_net_changes(), we'd do something like:
>
>   return context()->get_reporter()->diff_has_net_changes(*this);
>
> That way, the actual choice of what constitute the set of net changes
> would be left to the each reporter.
>
> Does that make any sense?

It does, but I was really overthinking things and had missed that the
leaf report headers are already different and already include the
extra missing bit of information needed to make things work with all
current tests. This is net_num_type_changes.

I'll send an updated patch. I would like to improve test coverage and
look at the other related functions (has_changes etc.), but this issue
is causing pain right now.

Regards,
Giuliano.

> [...]
>
>
> > (mostly but not all filtered out in non-leaf mode which may be its own
> > bug).
>
> Hopefully with this approach, this issue should go away as well.
>
> Thanks a lot for looking into this.
>
> Cheers,
>
> --
>                 Dodji
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>

[PATCH v2] Fix has_net_changes for --leaf-changes-only mode.

[Giuliano Procida]

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

    - Stats counters for changed variables and types have
      different names in the different modes.
    - 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, including some existing tests, the combination of
both issues still resulted in the correct return code.

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.

	* src/abg-comparison.cc
	(corpus_diff::priv::emit_diff_stats): Code whitespace.
	(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.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 src/abg-comparison.cc | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index 46bf9e30..50791cb5 100644
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -9932,7 +9932,7 @@ corpus_diff::priv::emit_diff_stats(const diff_stats&	s,
 	out << " (" << s.num_changed_func_filtered_out() << " filtered out)";
       out << ", ";
 
-      out << s.net_num_func_added()<< " Added";
+      out << s.net_num_func_added() << " Added";
       if (s.num_added_func_filtered_out())
 	out << " (" << s.num_added_func_filtered_out() << " filtered out)";
       if (total_nb_function_changes <= 1)
@@ -10560,7 +10560,7 @@ corpus_diff::has_incompatible_changes() const
 	  || stats.net_num_func_removed() != 0
 	  || (stats.num_func_with_virtual_offset_changes() != 0
 	      // If all reports about functions with sub-type changes
-	      // have been suppressd, then even those about functions
+	      // have been suppressed, then even those about functions
 	      // that are virtual don't matter anymore because the
 	      // user willingly requested to shut them down
 	      && stats.net_num_func_changed() != 0)
@@ -10602,21 +10602,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();
     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()
+	    || (leaf && stats.num_leaf_type_changes())
+	    || (leaf ? stats.net_num_leaf_func_changes()
+		     : stats.net_num_func_changed())
+	    || stats.net_num_func_added()
 	    || stats.net_num_vars_removed()
-	    || stats.net_num_removed_var_syms()
-	    || stats.net_num_added_unreachable_types()
+	    || (leaf ? stats.net_num_leaf_var_changes()
+		     : 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_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());
 }
 
 /// Apply the different filters that are registered to be applied to
-- 
2.26.0.rc2.310.g2932bb562d-goog

Re: [PATCH v2] Fix has_net_changes for --leaf-changes-only mode.

[Matthias Maennich]

On Fri, Mar 27, 2020 at 07:01:35PM +0000, Giuliano Procida wrote:
>This function was not aware of --leaf-changes-only mode.
>
>    - Stats counters for changed variables and types have
>      different names in the different modes.
>    - 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, including some existing tests, the combination of
>both issues still resulted in the correct return code.
>
>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.
>
>	* src/abg-comparison.cc
>	(corpus_diff::priv::emit_diff_stats): Code whitespace.
>	(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.
>
>Signed-off-by: Giuliano Procida <gprocida@google.com>

Would feel better to have some tests covering this along with the patch.
But this still looks correct to me.

Reviewed-by: Matthias Maennich <maennich@google.com>

Cheers,
Matthias

>---
> src/abg-comparison.cc | 30 ++++++++++++++++++------------
> 1 file changed, 18 insertions(+), 12 deletions(-)
>
>diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
>index 46bf9e30..50791cb5 100644
>--- a/src/abg-comparison.cc
>+++ b/src/abg-comparison.cc
>@@ -9932,7 +9932,7 @@ corpus_diff::priv::emit_diff_stats(const diff_stats&	s,
> 	out << " (" << s.num_changed_func_filtered_out() << " filtered out)";
>       out << ", ";
>
>-      out << s.net_num_func_added()<< " Added";
>+      out << s.net_num_func_added() << " Added";
>       if (s.num_added_func_filtered_out())
> 	out << " (" << s.num_added_func_filtered_out() << " filtered out)";
>       if (total_nb_function_changes <= 1)
>@@ -10560,7 +10560,7 @@ corpus_diff::has_incompatible_changes() const
> 	  || stats.net_num_func_removed() != 0
> 	  || (stats.num_func_with_virtual_offset_changes() != 0
> 	      // If all reports about functions with sub-type changes
>-	      // have been suppressd, then even those about functions
>+	      // have been suppressed, then even those about functions
> 	      // that are virtual don't matter anymore because the
> 	      // user willingly requested to shut them down
> 	      && stats.net_num_func_changed() != 0)
>@@ -10602,21 +10602,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();
>     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()
>+	    || (leaf && stats.num_leaf_type_changes())
>+	    || (leaf ? stats.net_num_leaf_func_changes()
>+		     : stats.net_num_func_changed())
>+	    || stats.net_num_func_added()
> 	    || stats.net_num_vars_removed()
>-	    || stats.net_num_removed_var_syms()
>-	    || stats.net_num_added_unreachable_types()
>+	    || (leaf ? stats.net_num_leaf_var_changes()
>+		     : 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_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());
> }
>
> /// Apply the different filters that are registered to be applied to
>-- 
>2.26.0.rc2.310.g2932bb562d-goog
>

[PATCH v3 0/1] Fix abidiff exit code when diffs are suppressed

[Giuliano Procida]

This is an update for a patch sent for review a while ago.

We're been running with the previous version for a while and
discovered the fix was incomplete. This version includes tests (as
requested in review) to verify the expected behaviour.

Regards,
Giuliano.

Giuliano Procida (1):
  Fix has_net_changes for --leaf-changes-only mode

 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

-- 
2.27.0.212.ge8ba1cc988-goog

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

[Giuliano Procida]

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

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();
     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()
+	    || (leaf && stats.net_num_leaf_type_changes())
+	    || (leaf ? stats.net_num_leaf_func_changes()
+		     : stats.net_num_func_changed())
+	    || stats.net_num_func_added()
 	    || stats.net_num_vars_removed()
-	    || stats.net_num_removed_var_syms()
-	    || stats.net_num_added_unreachable_types()
+	    || (leaf ? stats.net_num_leaf_var_changes()
+		     : 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_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());
 }
 
 /// Apply the different filters that are registered to be applied to
diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index 8ccd50a0..539644b1 100644
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -158,6 +158,15 @@ test-abidiff-exit/test-fun-param-v0.o \
 test-abidiff-exit/test-fun-param-v1.abi \
 test-abidiff-exit/test-fun-param-v1.c \
 test-abidiff-exit/test-fun-param-v1.o \
+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 00000000..66712b01
--- /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 00000000..4dd60963
--- /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 00000000..ca3b3e05
--- /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 00000000..4856a77a
--- /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 00000000..684b9c67
--- /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 00000000..7b2a4ad7
--- /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 00000000..6ba21189
--- /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 4d9c1943..c04b5835 100644
--- a/tests/test-abidiff-exit.cc
+++ b/tests/test-abidiff-exit.cc
@@ -212,6 +212,44 @@ InOutSpec in_out_specs[] =
     "data/test-abidiff-exit/test-fun-param-report.txt",
     "output/test-abidiff-exit/test-fun-param-report.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}
 };
 
-- 
2.27.0.212.ge8ba1cc988-goog

Re: [PATCH v3 1/1] Fix has_net_changes for --leaf-changes-only mode

[Dodji Seketeli]

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}
 };
 
-- 
1.8.3.1


-- 
		Dodji

Re: [PATCH v3 1/1] Fix has_net_changes for --leaf-changes-only mode

[Giuliano Procida]

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.

Re: [PATCH v3 1/1] Fix has_net_changes for --leaf-changes-only mode

[Dodji Seketeli]

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,

-- 
		Dodji