[PATCH v4 14/15] abg-suppression.cc: More consistent regex matching.
Matthias Maennich
maennich@google.com
Mon May 4 13:17:15 GMT 2020
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
>
More information about the Libabigail
mailing list