From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-il1-x143.google.com (mail-il1-x143.google.com [IPv6:2607:f8b0:4864:20::143]) by sourceware.org (Postfix) with ESMTPS id 8E9E6396EC6A for ; Fri, 2 Oct 2020 07:20:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8E9E6396EC6A Received: by mail-il1-x143.google.com with SMTP id b12so383281ilh.12 for ; Fri, 02 Oct 2020 00:20:53 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=VFUFI2VoVoFnPC6ceftkgfMOlUxCPBtzVUltpJQOnA0=; b=VVmVQXllSnKX17C9yVvhyCjwmLrkB5w9dTtTZPmrymSOaN/jCzO2iI2a0TincXF7wE 46Uw87g4I+XvmscJSZVJnYhw3n+lr7vre1ZGjXW/KcCVIqZqv0cWBg7uxdVQoxQKmLz+ xZ+tfsF7tdDL1fFodb+RFtWlGdRHzMF0/mZMRPjD0AOV4SM8ME7LSWyCWJBESEK326hu HQs0MaeOB3VrvFJPdJqsqsjBC8D0xE57kKBjoS1O2lbFAIwU0StbCZRo3UnM+Jtm93H8 CgqGIuk8vOWEpmdp58nULQIKboar3ijVVtI5+Hy1ecwBbcQynd29hUq40tLTAF5IkRqy EkKw== X-Gm-Message-State: AOAM533rxOwCj/Axd5RLS0Di98oiknviYlrX61OlAEu4bFkw96ImTmPa vQZtPdO/bEPKdKDnSuKWdDTCAzB3GR0lf0077IXtng== X-Google-Smtp-Source: ABdhPJzsyCus8P3KAWH/fdMjx9A2XActh1eCGlj9g6T82szaUU5mEkWr47sRLr9dIgEWBbdI3cxxEzIEt4Ax+escOVI= X-Received: by 2002:a92:6b0d:: with SMTP id g13mr861392ilc.242.1601623252836; Fri, 02 Oct 2020 00:20:52 -0700 (PDT) MIME-Version: 1.0 References: <20200817093819.172380-1-gprocida@google.com> <20200817093819.172380-4-gprocida@google.com> <87o8lmos73.fsf@seketeli.org> In-Reply-To: <87o8lmos73.fsf@seketeli.org> From: Giuliano Procida Date: Fri, 2 Oct 2020 08:20:16 +0100 Message-ID: Subject: Re: [PATCH 3/7] Better suppression section parsing delegation. To: Dodji Seketeli Cc: libabigail@sourceware.org, kernel-team@android.com, =?UTF-8?Q?Matthias_M=C3=A4nnich?= X-Spam-Status: No, score=-28.0 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, GIT_PATCH_0, HTML_MESSAGE, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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: Fri, 02 Oct 2020 07:20:55 -0000 Hi Dodji. This is the start of a sequence of much longer changes to the suppression code that make the parser into a real entity in its own right. https://github.com/myxoid/libabigail/commits/suppression-parsing-error-hand= ling gets to the point of having a table-driven parser where error status is threaded to the top-level (and is followed by further series). The redundancy of error checking you suggest could be kept but would be somewhat out of place in the table-driven parser. If you can see a way that these changes might be useful, perhaps with small or large changes, let me know. Otherwise it may be time to abandon the effort. Regards, Giuliano. On Thu, 1 Oct 2020 at 16:56, Dodji Seketeli wrote: > 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. > > > > > /// Read suppressions specifications from an input stream. > > @@ -1564,9 +1572,6 @@ read_type_suppression(const ini::config::section& > section) > > { > > type_suppression_sptr result; > > > > - 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, > > -- > Dodji >