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

[Dodji Seketeli <dodji@seketeli.org>]

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