[3/7] Better suppression section parsing delegation.
Commit Message
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.
This can be simplified.
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(-)
Comments
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,
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
>
@@ -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);
-
+ {
+ 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);
+ }
}
/// 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;
-
static const char *const sufficient_props[] = {
"file_name_regexp",
"file_name_not_regexp",
@@ -3153,9 +3158,6 @@ read_function_suppression(const ini::config::section& section)
{
function_suppression_sptr result;
- if (section.get_name() != "suppress_function")
- return result;
-
static const char *const sufficient_props[] = {
"label",
"file_name_regexp",
@@ -4033,9 +4035,6 @@ read_variable_suppression(const ini::config::section& section)
{
variable_suppression_sptr result;
- if (section.get_name() != "suppress_variable")
- return result;
-
static const char *const sufficient_props[] = {
"label",
"file_name_regexp",
@@ -4298,9 +4297,6 @@ read_file_suppression(const ini::config::section& section)
{
file_suppression_sptr result;
- if (section.get_name() != "suppress_file")
- return result;
-
static const char *const sufficient_props[] = {
"file_name_regexp",
"file_name_not_regexp",