From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net [217.70.183.196]) by sourceware.org (Postfix) with ESMTPS id 7BDDD3857C6D for ; Thu, 1 Oct 2020 15:56:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 7BDDD3857C6D 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 X-Originating-IP: 91.166.131.130 Received: from localhost (91-166-131-130.subs.proxad.net [91.166.131.130]) (Authenticated sender: dodji@seketeli.org) by relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 889D5E0019; Thu, 1 Oct 2020 15:56:49 +0000 (UTC) Received: by localhost (Postfix, from userid 1000) id 224661800938; Thu, 1 Oct 2020 17:56:48 +0200 (CEST) From: Dodji Seketeli To: Giuliano Procida Cc: libabigail@sourceware.org, kernel-team@android.com Subject: Re: [PATCH 3/7] Better suppression section parsing delegation. Organization: Me, myself and I References: <20200817093819.172380-1-gprocida@google.com> <20200817093819.172380-4-gprocida@google.com> X-Operating-System: Red Hat Enterprise Linux Workstation 7.8 Beta X-URL: http://www.seketeli.net/~dodji Date: Thu, 01 Oct 2020 17:56:48 +0200 In-Reply-To: <20200817093819.172380-4-gprocida@google.com> (Giuliano Procida's message of "Mon, 17 Aug 2020 10:38:15 +0100") Message-ID: <87o8lmos73.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=-8.2 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, 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: Thu, 01 Oct 2020 15:56:53 -0000 Giuliano Procida a =C3=A9crit: > The function read_suppressions hands the same ini config section to > each of four parsing functions, until one succeeds. Each of those in > turn has an early exit if the name of the section doesn't correspond. Another way of seeing it is that the section is read either as type, function, variable or file suppression. > This can be simplified. It's not obvious to me that the result you are proposing below is "simpler" per se. It seems to me that this just boils down to a matter of taste. > > This patch moves the name checking into read_suppressions so that > exactly one parsing function is called per section. > > * src/abg-suppression.cc (read_suppressions): Call appropriate > read_foo_suppression function based on section name. > (read_type_suppression): Remove section name check. > (read_function_suppression): Ditto. > (read_variable_suppression): Ditto. > (read_file_suppression): Ditto. > > Signed-off-by: Giuliano Procida > --- > src/abg-suppression.cc | 34 +++++++++++++++------------------- > 1 file changed, 15 insertions(+), 19 deletions(-) > > diff --git a/src/abg-suppression.cc b/src/abg-suppression.cc > index ae7cc95ce..682bb8742 100644 > --- a/src/abg-suppression.cc > +++ b/src/abg-suppression.cc > @@ -372,17 +372,25 @@ static void > read_suppressions(const ini::config& config, > suppressions_type& suppressions) > { > - suppression_sptr s; > for (ini::config::sections_type::const_iterator i =3D > config.get_sections().begin(); > i !=3D config.get_sections().end(); > ++i) > - if ((s =3D read_type_suppression(**i)) > - || (s =3D read_function_suppression(**i)) > - || (s =3D read_variable_suppression(**i)) > - || (s =3D read_file_suppression(**i))) > - suppressions.push_back(s); > - As I was saying earlier, the above can be understood as "the section is read either as a type, function, variable or file suppression." > + { > + const ini::config::section_sptr& section =3D *i; > + const std::string& name =3D section->get_name(); > + suppression_sptr s; > + if (name =3D=3D "suppress_type") > + s =3D read_type_suppression(*section); > + else if (name =3D=3D "suppress_function") > + s =3D read_function_suppression(*section); > + else if (name =3D=3D "suppress_variable") > + s =3D read_variable_suppression(*section); > + else if (name =3D=3D "suppress_file") > + s =3D read_file_suppression(*section); > + if (s) > + suppressions.push_back(s); > + } > } I don't see how this is simpler than the initial code, quite frankly. >=20=20 > /// Read suppressions specifications from an input stream. > @@ -1564,9 +1572,6 @@ read_type_suppression(const ini::config::section& s= ection) > { > type_suppression_sptr result; >=20=20 > - if (section.get_name() !=3D "suppress_type") > - return result; > - Actually, I'd prefer making this function return early when wrongly called on a section that is not a type suppression. So I think removing this check actually makes the function less robust. I have a similar comment for the changes to the other parsing functions below this one. [...] Cheers, --=20 Dodji