From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay12.mail.gandi.net (relay12.mail.gandi.net [217.70.178.232]) by sourceware.org (Postfix) with ESMTPS id F33D13858D38 for ; Tue, 21 Jul 2020 06:28:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org F33D13858D38 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=seketeli.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=dodji@seketeli.org Received: from localhost (91-166-131-130.subs.proxad.net [91.166.131.130]) (Authenticated sender: dodji@seketeli.org) by relay12.mail.gandi.net (Postfix) with ESMTPSA id 53714200003; Tue, 21 Jul 2020 06:28:18 +0000 (UTC) Received: by localhost (Postfix, from userid 1000) id B0C0F18009B1; Tue, 21 Jul 2020 08:28:16 +0200 (CEST) From: Dodji Seketeli To: Giuliano Procida Cc: libabigail@sourceware.org, kernel-team@android.com, Matthias =?utf-8?Q?M=C3=A4nnich?= Subject: Re: [PATCH v3 1/1] Fix has_net_changes for --leaf-changes-only mode Organization: Me, myself and I References: <20200330091754.GH101337@google.com> <20200701131907.2665674-1-gprocida@google.com> <20200701131907.2665674-2-gprocida@google.com> <87a6zyi6vq.fsf@seketeli.org> X-Operating-System: Red Hat Enterprise Linux Workstation 7.8 Beta X-URL: http://www.seketeli.net/~dodji Date: Tue, 21 Jul 2020 08:28:16 +0200 In-Reply-To: (Giuliano Procida's message of "Fri, 17 Jul 2020 17:34:59 +0100") Message-ID: <87wo2xgz5r.fsf@seketeli.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-3.5 required=5.0 tests=BAYES_00, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Mailing list of the Libabigail project List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 21 Jul 2020 06:28:22 -0000 Giuliano Procida a =C3=A9crit: [...] > 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 =3D const_cast(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 =3D const_cast(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, --=20 Dodji