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

[Dodji Seketeli <dodji@seketeli.org>]

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