[v4,14/15] abg-suppression.cc: More consistent regex matching.

  Following previous patches, various minor inconsistencies in the
suppression regex matching code were more readily apparent.

- regex_t_sptr values are fetched both by reference and by
  value (which has a reference count mutation cost)
- there's a mixture of code styles for testing regex presence
    - regex = ...; if (regex && ...)
    - if (regex = ...) if (...)
  the latter has the advantage that the variable has a smaller scope
  and can be given a shorter name
- in some cases there are redundant tests (always true due to previous
  conditional logic)

This patch ensures all shared pointers to compiled regexes are fetched
by const reference and that the code uses the nested if style of
presence checking where possible. It simplifies some logic where
there's redundancy.

There are no behavioural changes. There may be performance
improvements.

	* src/abg-suppression.cc (suppression_matches_type_name): Get
	regexes by const reference.
	(suppression_matches_type_location): Get regexes by const
	reference. (function_suppression::suppresses_function): Get
	regexes by const reference; use nested if checks; simplify
	logic around symbol version checks.
	(function_suppression::suppresses_function_symbol): Get
	regexes by const reference; use nested if checks; remove
	redundant regex presence checks.
	(suppression_matches_function_name): Get regexes by const
	reference. (suppression_matches_function_sym_name): Get
	regexes by const reference.
	(suppression_matches_variable_name): Get regexes by const
	reference. (suppression_matches_variable_sym_name): Get
	regexes by const reference. (suppression_matches_type): Get
	regexes by const reference.
	(variable_suppression::suppresses_variable): Get regexes by
	const reference; use nested if checks; remove redundant
	type_name empty check.
	(variable_suppression::suppresses_variable_symbol): Get
	regexes by const reference; use nested if checks.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 src/abg-suppression.cc | 167 +++++++++++++++++++----------------------
 1 file changed, 76 insertions(+), 91 deletions(-)
  

On Mon, May 04, 2020 at 01:34:15PM +0100, Giuliano Procida wrote:
>Following previous patches, various minor inconsistencies in the
>suppression regex matching code were more readily apparent.
>
>- regex_t_sptr values are fetched both by reference and by
>  value (which has a reference count mutation cost)
>- there's a mixture of code styles for testing regex presence
>    - regex = ...; if (regex && ...)
>    - if (regex = ...) if (...)
>  the latter has the advantage that the variable has a smaller scope
>  and can be given a shorter name
>- in some cases there are redundant tests (always true due to previous
>  conditional logic)
>
>This patch ensures all shared pointers to compiled regexes are fetched
>by const reference and that the code uses the nested if style of
>presence checking where possible. It simplifies some logic where
>there's redundancy.
>
>There are no behavioural changes. There may be performance
>improvements.
>
>	* src/abg-suppression.cc (suppression_matches_type_name): Get
>	regexes by const reference.
>	(suppression_matches_type_location): Get regexes by const
>	reference. (function_suppression::suppresses_function): Get
>	regexes by const reference; use nested if checks; simplify
>	logic around symbol version checks.
>	(function_suppression::suppresses_function_symbol): Get
>	regexes by const reference; use nested if checks; remove
>	redundant regex presence checks.
>	(suppression_matches_function_name): Get regexes by const
>	reference. (suppression_matches_function_sym_name): Get
>	regexes by const reference.
>	(suppression_matches_variable_name): Get regexes by const
>	reference. (suppression_matches_variable_sym_name): Get
>	regexes by const reference. (suppression_matches_type): Get
>	regexes by const reference.
>	(variable_suppression::suppresses_variable): Get regexes by
>	const reference; use nested if checks; remove redundant
>	type_name empty check.
>	(variable_suppression::suppresses_variable_symbol): Get
>	regexes by const reference; use nested if checks.
>
>Signed-off-by: Giuliano Procida <gprocida@google.com>

Reviewed-by: Matthias Maennich <maennich@google.com>

Cheers,
Matthias
>---
> src/abg-suppression.cc | 167 +++++++++++++++++++----------------------
> 1 file changed, 76 insertions(+), 91 deletions(-)
>
>diff --git a/src/abg-suppression.cc b/src/abg-suppression.cc
>index 4883168d..24f75b9a 100644
>--- a/src/abg-suppression.cc
>+++ b/src/abg-suppression.cc
>@@ -1049,15 +1049,14 @@ suppression_matches_type_name(const type_suppression&	s,
> 	  // If the qualified name of the considered type doesn't match
> 	  // the regular expression of the type name, then this
> 	  // suppression doesn't apply.
>-	  if (const regex_t_sptr& type_name_regex =
>-	      s.get_type_name_regex())
>+	  if (const regex_t_sptr& type_name_regex = s.get_type_name_regex())
> 	    {
> 	      if (!regex::match(type_name_regex, type_name))
> 		return false;
> 	    }
>
>-	  if (const regex_t_sptr type_name_not_regex =
>-	      s.get_type_name_not_regex())
>+	  if (const regex_t_sptr& type_name_not_regex =
>+		s.get_type_name_not_regex())
> 	    {
> 	      if (regex::match(type_name_not_regex, type_name))
> 		return false;
>@@ -1105,7 +1104,7 @@ suppression_matches_type_location(const type_suppression&	s,
>       unsigned loc_line = 0, loc_column = 0;
>       loc.expand(loc_path, loc_line, loc_column);
>
>-      if (regex_t_sptr regexp = s.get_source_location_to_keep_regex())
>+      if (const regex_t_sptr& regexp = s.get_source_location_to_keep_regex())
> 	if (regex::match(regexp, loc_path))
> 	  return false;
>
>@@ -2544,7 +2543,7 @@ function_suppression::suppresses_function(const function_decl* fn,
>     }
>
>   // check if the "name_regexp" property matches.
>-  const regex_t_sptr name_regex = get_name_regex();
>+  const regex_t_sptr& name_regex = get_name_regex();
>   if (name_regex)
>     {
>       if (!regex::match(name_regex, fname))
>@@ -2575,7 +2574,7 @@ function_suppression::suppresses_function(const function_decl* fn,
>     }
>
>   // check if the "name_not_regexp" property matches.
>-  const regex_t_sptr name_not_regex = get_name_not_regex();
>+  const regex_t_sptr& name_not_regex = get_name_not_regex();
>   if (name_not_regex)
>     {
>       if (regex::match(name_not_regex, fname))
>@@ -2619,11 +2618,9 @@ function_suppression::suppresses_function(const function_decl* fn,
>       if (fn_return_type_name != get_return_type_name())
> 	return false;
>     }
>-  else
>+  else if (const regex_t_sptr& regex = get_return_type_regex())
>     {
>-      const regex_t_sptr return_type_regex = get_return_type_regex();
>-      if (return_type_regex
>-	  && !regex::match(return_type_regex, fn_return_type_name))
>+      if (!regex::match(regex, fn_return_type_name))
> 	return false;
>     }
>
>@@ -2659,14 +2656,15 @@ function_suppression::suppresses_function(const function_decl* fn,
>     }
>   else if (sym)
>     {
>-      const regex_t_sptr symbol_name_regex = get_symbol_name_regex();
>-      if (symbol_name_regex && !regex::match(symbol_name_regex, fn_sym_name))
>-	return false;
>+      const regex_t_sptr& symbol_name_regex = get_symbol_name_regex();
>+      const regex_t_sptr& symbol_name_not_regex = get_symbol_name_not_regex();
>
>-      const regex_t_sptr symbol_name_not_regex = get_symbol_name_not_regex();
>-      if (symbol_name_not_regex
>-	  && regex::match(symbol_name_not_regex, fn_sym_name))
>-	return false;
>+      if (symbol_name_regex)
>+	if (!regex::match(symbol_name_regex, fn_sym_name))
>+	  return false;
>+      if (symbol_name_not_regex)
>+	if (regex::match(symbol_name_not_regex, fn_sym_name))
>+	  return false;
>
>       if (get_allow_other_aliases())
> 	{
>@@ -2679,13 +2677,13 @@ function_suppression::suppresses_function(const function_decl* fn,
> 		   a && !a->is_main_symbol();
> 		   a = a->get_next_alias())
> 		{
>-		  if (symbol_name_regex
>-		      && !regex::match(symbol_name_regex, a->get_name()))
>-		    return false;
>-
>-		  if (symbol_name_not_regex
>-		      && regex::match(symbol_name_not_regex, a->get_name()))
>-		    return false;
>+		  const std::string& alias_name = a->get_name();
>+		  if (symbol_name_regex)
>+		    if (!regex::match(symbol_name_regex, alias_name))
>+		      return false;
>+		  if (symbol_name_not_regex)
>+		    if (regex::match(symbol_name_not_regex, alias_name))
>+		      return false;
> 		}
> 	    }
> 	}
>@@ -2693,17 +2691,19 @@ function_suppression::suppresses_function(const function_decl* fn,
>
>   // Check if the "symbol_version" and "symbol_version_regexp"
>   // properties match.
>-  if (sym && !get_symbol_version().empty())
>-    {
>-      if (fn_sym_version != get_symbol_version())
>-	return false;
>-    }
>-  else if (sym)
>+  if (sym)
>     {
>-      const regex_t_sptr symbol_version_regex = get_symbol_version_regex();
>-      if (symbol_version_regex
>-	  && !regex::match(symbol_version_regex, fn_sym_version))
>-	return false;
>+      if (!get_symbol_version().empty())
>+	{
>+	  if (fn_sym_version != get_symbol_version())
>+	    return false;
>+	}
>+      else
>+	{
>+	  if (const regex_t_sptr& regex = get_symbol_version_regex())
>+	    if (!regex::match(regex, fn_sym_version))
>+	      return false;
>+	}
>     }
>
>   // Check the 'parameter' property.
>@@ -2737,16 +2737,11 @@ function_suppression::suppresses_function(const function_decl* fn,
> 	      if (tn != fn_parm_type_qualified_name)
> 		return false;
> 	    }
>-	  else
>+	  else if (const regex_t_sptr& regex =
>+		     (*p)->get_parameter_type_name_regex())
> 	    {
>-	      const regex_t_sptr parm_type_name_regex =
>-		(*p)->get_parameter_type_name_regex();
>-	      if (parm_type_name_regex)
>-		{
>-		  if (!regex::match(parm_type_name_regex,
>-				    fn_parm_type_qualified_name))
>-		    return false;
>-		}
>+	      if (!regex::match(regex, fn_parm_type_qualified_name))
>+		return false;
> 	    }
> 	}
>     }
>@@ -2829,10 +2824,9 @@ function_suppression::suppresses_function_symbol(const elf_symbol* sym,
>       if (sym_name != get_symbol_name())
> 	return false;
>     }
>-  else if (get_symbol_name_regex())
>+  else if (const regex_t_sptr& regex = get_symbol_name_regex())
>     {
>-      const regex_t_sptr symbol_name_regex = get_symbol_name_regex();
>-      if (symbol_name_regex && !regex::match(symbol_name_regex, sym_name))
>+      if (!regex::match(regex, sym_name))
> 	return false;
>     }
>   else
>@@ -2844,11 +2838,9 @@ function_suppression::suppresses_function_symbol(const elf_symbol* sym,
>       if (sym_version != get_symbol_version())
> 	return false;
>     }
>-  else if (get_symbol_version_regex())
>+  else if (const regex_t_sptr& regex = get_symbol_version_regex())
>     {
>-      const regex_t_sptr symbol_version_regex = get_symbol_version_regex();
>-      if (symbol_version_regex
>-	  && !regex::match(symbol_version_regex, sym_version))
>+      if (!regex::match(regex, sym_version))
> 	return false;
>     }
>   else
>@@ -2938,12 +2930,12 @@ bool
> suppression_matches_function_name(const suppr::function_suppression& s,
> 				  const string& fn_name)
> {
>-  if (regex_t_sptr regexp = s.get_name_regex())
>+  if (const regex_t_sptr& regexp = s.get_name_regex())
>     {
>       if (!regex::match(regexp, fn_name))
> 	return false;
>     }
>-  else if (regex_t_sptr regexp = s.get_name_not_regex())
>+  else if (const regex_t_sptr& regexp = s.get_name_not_regex())
>     {
>       if (regex::match(regexp, fn_name))
> 	return false;
>@@ -2974,12 +2966,12 @@ bool
> suppression_matches_function_sym_name(const suppr::function_suppression& s,
> 				      const string& fn_linkage_name)
> {
>-  if (regex_t_sptr regexp = s.get_symbol_name_regex())
>+  if (const regex_t_sptr& regexp = s.get_symbol_name_regex())
>     {
>       if (!regex::match(regexp, fn_linkage_name))
> 	return false;
>     }
>-  else if (regex_t_sptr regexp = s.get_symbol_name_not_regex())
>+  else if (const regex_t_sptr& regexp = s.get_symbol_name_not_regex())
>     {
>       if (regex::match(regexp, fn_linkage_name))
> 	return false;
>@@ -3007,12 +2999,12 @@ bool
> suppression_matches_variable_name(const suppr::variable_suppression& s,
> 				  const string& var_name)
> {
>-  if (regex_t_sptr regexp = s.get_name_regex())
>+  if (const regex_t_sptr& regexp = s.get_name_regex())
>     {
>       if (!regex::match(regexp, var_name))
> 	return false;
>     }
>-  else if (regex_t_sptr regexp = s.get_name_not_regex())
>+  else if (const regex_t_sptr& regexp = s.get_name_not_regex())
>     {
>       if (regex::match(regexp, var_name))
> 	return false;
>@@ -3041,12 +3033,12 @@ bool
> suppression_matches_variable_sym_name(const suppr::variable_suppression& s,
> 				      const string& var_linkage_name)
> {
>-  if (regex_t_sptr regexp = s.get_symbol_name_regex())
>+  if (const regex_t_sptr& regexp = s.get_symbol_name_regex())
>     {
>       if (!regex::match(regexp, var_linkage_name))
> 	return false;
>     }
>-  else if (regex_t_sptr regexp = s.get_symbol_name_not_regex())
>+  else if (const regex_t_sptr& regexp = s.get_symbol_name_not_regex())
>     {
>       if (regex::match(regexp, var_linkage_name))
> 	return false;
>@@ -3075,7 +3067,7 @@ bool
> suppression_matches_type(const suppr::type_suppression& s,
> 			 const string& type_name)
> {
>-  if (regex_t_sptr regexp = s.get_type_name_regex())
>+  if (const regex_t_sptr& regexp = s.get_type_name_regex())
>     {
>       if (!regex::match(regexp, type_name))
> 	return false;
>@@ -3784,13 +3776,13 @@ variable_suppression::suppresses_variable(const var_decl* var,
>       // "name_regex" and "name_not_regex" properties match
>       if (get_name().empty())
> 	{
>-	  const regex_t_sptr name_regex = get_name_regex();
>-	  if (name_regex && !regex::match(name_regex, var_name))
>-	    return false;
>+	  if (const regex_t_sptr& regex = get_name_regex())
>+	    if (!regex::match(regex, var_name))
>+	      return false;
>
>-	  const regex_t_sptr name_not_regex = get_name_not_regex();
>-	  if (name_not_regex && regex::match(name_not_regex, var_name))
>-	    return false;
>+	  if (const regex_t_sptr& regex = get_name_not_regex())
>+	    if (regex::match(regex, var_name))
>+	      return false;
> 	}
>     }
>
>@@ -3804,13 +3796,13 @@ variable_suppression::suppresses_variable(const var_decl* var,
>     }
>   else
>     {
>-      const regex_t_sptr sym_name_regex = get_symbol_name_regex();
>-      if (sym_name_regex && !regex::match(sym_name_regex, var_sym_name))
>-	return false;
>+      if (const regex_t_sptr& regex = get_symbol_name_regex())
>+	if (!regex::match(regex, var_sym_name))
>+	  return false;
>
>-      const regex_t_sptr sym_name_not_regex = get_symbol_name_not_regex();
>-      if (sym_name_not_regex && regex::match(sym_name_not_regex, var_sym_name))
>-	return false;
>+      if (const regex_t_sptr& regex = get_symbol_name_not_regex())
>+	if (regex::match(regex, var_sym_name))
>+	  return false;
>     }
>
>   // Check for symbol_version and symbol_version_regexp property match
>@@ -3823,10 +3815,9 @@ variable_suppression::suppresses_variable(const var_decl* var,
>     }
>   else
>     {
>-      const regex_t_sptr symbol_version_regex = get_symbol_version_regex();
>-      if (symbol_version_regex
>-	  && !regex::match(symbol_version_regex, var_sym_version))
>-	return false;
>+      if (const regex_t_sptr& regex = get_symbol_version_regex())
>+	if (!regex::match(regex, var_sym_version))
>+	  return false;
>     }
>
>   // Check for the "type_name" and type_name_regex properties match.
>@@ -3840,12 +3831,9 @@ variable_suppression::suppresses_variable(const var_decl* var,
>     }
>   else
>     {
>-      if (get_type_name().empty())
>-	{
>-	  const regex_t_sptr type_name_regex = get_type_name_regex();
>-	  if (type_name_regex && !regex::match(type_name_regex, var_type_name))
>-	    return false;
>-	}
>+      if (const regex_t_sptr& regex = get_type_name_regex())
>+	if (!regex::match(regex, var_type_name))
>+	  return false;
>     }
>
>   return true;
>@@ -3931,10 +3919,9 @@ variable_suppression::suppresses_variable_symbol(const elf_symbol* sym,
>       if (get_symbol_name() != sym_name)
> 	return false;
>     }
>-  else if (get_symbol_name_regex())
>+  else if (const regex_t_sptr& regex = get_symbol_name_regex())
>     {
>-      const regex_t_sptr sym_name_regex = get_symbol_name_regex();
>-      if (sym_name_regex && !regex::match(sym_name_regex, sym_name))
>+      if (!regex::match(regex, sym_name))
> 	return false;
>     }
>   else
>@@ -3946,11 +3933,9 @@ variable_suppression::suppresses_variable_symbol(const elf_symbol* sym,
>       if (get_symbol_version() != sym_version)
> 	return false;
>     }
>-  else if (get_symbol_version_regex())
>+  else if (const regex_t_sptr& regex = get_symbol_version_regex())
>     {
>-      const regex_t_sptr symbol_version_regex = get_symbol_version_regex();
>-      if (symbol_version_regex
>-	  && !regex::match(symbol_version_regex, sym_version))
>+      if (!regex::match(regex, sym_version))
> 	return false;
>     }
>   else
>@@ -4251,14 +4236,14 @@ file_suppression::suppresses_file(const string& file_path)
>
>   bool has_regexp = false;
>
>-  if (regex_t_sptr regexp = get_file_name_regex())
>+  if (const regex_t_sptr& regexp = get_file_name_regex())
>     {
>       has_regexp = true;
>       if (!regex::match(regexp, fname))
> 	return false;
>     }
>
>-  if (regex_t_sptr regexp = get_file_name_not_regex())
>+  if (const regex_t_sptr& regexp = get_file_name_not_regex())
>     {
>       has_regexp = true;
>       if (regex::match(regexp, fname))
>-- 
>2.26.2.526.g744177e7f7-goog
>