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

[Dodji Seketeli <dodji@seketeli.org>]

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