Re: [PATCH 3/7] Better suppression section parsing delegation.

[Giuliano Procida <gprocida@google.com>]

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-handling
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 <dodji@seketeli.org> wrote:

> Giuliano Procida <gprocida@google.com> a écrit:
>
> > 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 <gprocida@google.com>
> > ---
> >  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 =
> >        config.get_sections().begin();
> >         i != config.get_sections().end();
> >         ++i)
> > -    if ((s = read_type_suppression(**i))
> > -     || (s = read_function_suppression(**i))
> > -     || (s = read_variable_suppression(**i))
> > -     || (s = 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 = *i;
> > +      const std::string& name = section->get_name();
> > +      suppression_sptr s;
> > +      if (name == "suppress_type")
> > +     s = read_type_suppression(*section);
> > +      else if (name == "suppress_function")
> > +     s = read_function_suppression(*section);
> > +      else if (name == "suppress_variable")
> > +     s = read_variable_suppression(*section);
> > +      else if (name == "suppress_file")
> > +     s = 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() != "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
>