Re: [PATCH v3 18/21] Refactor suppression property regex parsing.

[Giuliano Procida <gprocida@google.com>]

Hi.

On Mon, 27 Apr 2020 at 15:55, Matthias Maennich <maennich@google.com> wrote:
>
> On Fri, Apr 24, 2020 at 10:21:29AM +0100, Giuliano Procida wrote:
> >This patch introduces a helper function to look up a key in an ini
> >configuration section and get a compiled regex value. All regex
> >look-ups now use this.
> >
> >There are no behavioural changes.
> >
>
> That is an awesome simplification!
>
> I basically have the same comment as before: Getting different data
> types from the ini asks (in my opinion) for a bit generic code.

The generic code comes a bit later. We cannot do everything on a
purely type-driven basis (complications around
has_data_member_inserted* properties), but we can get quite close.

As I note in my comment for 17/21, this is the wrong refactoring. What
the code should look like is (once error handling is there as well):

if (prop prop = section.find_property("foo_regex")) {
  regex r;
  if (!read_regex(prop, r))
    return false;
  result.set_foo_regex(r);
}

or

if (prop prop = section.find_property("foo_regex"))
  if (!type_driven_magic(prop, &bar_suppression::set_foo_regex))
    return false;

for the general case we need (adapter can be "call" in almost all cases):

if (prop prop = section.find_property("foo_regex"))
  if (!type_driven_magic(prop, adapter(&bar_suppression::set_foo_regex)))
    return false;

And once it's table-driven, it's just:

bar_suppression_table = {
  { "foo_regex", { adapter(&bar_suppression::set_foo_regex), ... } },
  ...
};

I'm undecided as to whether the type-driven magic (calling the right
function to extract a type from a property) should be with template
specialisation or plain overloading.

> But I am also ok with this version.
>
> Reviewed-by: Matthias Maennich <maennich@google.com>

I'll hold off until further feedback on 17/21 and this one.

Thanks,
Giuliano.

> Cheers,
> Matthias
>
> >Signed-off-by: Giuliano Procida <gprocida@google.com>
> >---
> > src/abg-suppression.cc | 215 ++++++++++++-----------------------------
> > 1 file changed, 61 insertions(+), 154 deletions(-)
> >
> >diff --git a/src/abg-suppression.cc b/src/abg-suppression.cc
> >index 84c9b78f..8b3631fd 100644
> >--- a/src/abg-suppression.cc
> >+++ b/src/abg-suppression.cc
> >@@ -1619,6 +1619,30 @@ maybe_get_string_prop(const ini::config::section& section,
> >   return true;
> > }
> >
> >+/// Maybe compile a property to a regex.
> >+///
> >+/// Attempt to find a simple property in an ini section and compile it
> >+/// to a regex.
> >+///
> >+/// @param section the ini section to look in
> >+///
> >+/// @param prop the property name
> >+///
> >+/// @param r the regex to compile into
> >+///
> >+/// @return whether the property was found
> >+static bool
> >+maybe_get_regex_prop(const ini::config::section& section,
> >+                     const std::string& name,
> >+                     regex_t_sptr& regex)
> >+{
> >+  string str;
> >+  if (!maybe_get_string_prop(section, name, str))
> >+    return false;
> >+  regex = regex::compile(str);
> >+  return true;
> >+}
> >+
> > /// Read a type suppression from an instance of ini::config::section
> > /// and build a @ref type_suppression as a result.
> > ///
> >@@ -1641,45 +1665,23 @@ read_type_suppression(const ini::config::section& section)
> >   string label_str;
> >   maybe_get_string_prop(section, "label", label_str);
> >
> >-  ini::simple_property_sptr file_name_regex_prop =
> >-    is_simple_property(section.find_property("file_name_regexp"));
> >   regex_t_sptr file_name_regex;
> >-  if (file_name_regex_prop)
> >-    file_name_regex =
> >-      regex::compile(file_name_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "file_name_regexp", file_name_regex);
> >
> >-  ini::simple_property_sptr file_name_not_regex_prop =
> >-    is_simple_property(section.find_property("file_name_not_regexp"));
> >   regex_t_sptr file_name_not_regex;
> >-  if (file_name_not_regex_prop)
> >-    file_name_not_regex =
> >-      regex::compile(file_name_not_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "file_name_not_regexp", file_name_not_regex);
> >
> >-  ini::simple_property_sptr soname_regex_prop =
> >-    is_simple_property(section.find_property("soname_regexp"));
> >   regex_t_sptr soname_regex;
> >-  if (soname_regex_prop)
> >-    soname_regex = regex::compile(soname_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "soname_regexp", soname_regex);
> >
> >-  ini::simple_property_sptr soname_not_regex_prop =
> >-    is_simple_property(section.find_property("soname_not_regexp"));
> >   regex_t_sptr soname_not_regex;
> >-  if (soname_not_regex_prop)
> >-    soname_not_regex =
> >-      regex::compile(soname_not_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "soname_not_regexp", soname_not_regex);
> >
> >-  ini::simple_property_sptr name_regex_prop =
> >-    is_simple_property(section.find_property("name_regexp"));
> >   regex_t_sptr name_regex;
> >-  if (name_regex_prop)
> >-    name_regex = regex::compile(name_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "name_regexp", name_regex);
> >
> >-  ini::simple_property_sptr name_not_regex_prop =
> >-    is_simple_property(section.find_property("name_not_regexp"));
> >   regex_t_sptr name_not_regex;
> >-  if (name_not_regex_prop)
> >-    name_not_regex =
> >-      regex::compile(name_not_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "name_not_regexp", name_not_regex);
> >
> >   string name_str;
> >   maybe_get_string_prop(section, "name", name_str);
> >@@ -1706,12 +1708,8 @@ read_type_suppression(const ini::config::section& section)
> >       }
> >     }
> >
> >-  ini::simple_property_sptr srcloc_not_regexp_prop =
> >-    is_simple_property(section.find_property("source_location_not_regexp"));
> >   regex_t_sptr srcloc_not_regex;
> >-  if (srcloc_not_regexp_prop)
> >-    srcloc_not_regex =
> >-      regex::compile(srcloc_not_regexp_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "source_location_not_regexp", srcloc_not_regex);
> >
> >   std::string type_kind_str;
> >   bool consider_type_kind =
> >@@ -3195,85 +3193,47 @@ read_function_suppression(const ini::config::section& section)
> >   string label_str;
> >   maybe_get_string_prop(section, "label", label_str);
> >
> >-  ini::simple_property_sptr file_name_regex_prop =
> >-    is_simple_property(section.find_property("file_name_regexp"));
> >   regex_t_sptr file_name_regex;
> >-  if (file_name_regex_prop)
> >-    file_name_regex =
> >-      regex::compile(file_name_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "file_name_regexp", file_name_regex);
> >
> >-  ini::simple_property_sptr file_name_not_regex_prop =
> >-    is_simple_property(section.find_property("file_name_not_regexp"));
> >   regex_t_sptr file_name_not_regex;
> >-  if (file_name_not_regex_prop)
> >-    file_name_not_regex =
> >-      regex::compile(file_name_not_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "file_name_not_regexp", file_name_not_regex);
> >
> >-  ini::simple_property_sptr soname_regex_prop =
> >-    is_simple_property(section.find_property("soname_regexp"));
> >   regex_t_sptr soname_regex;
> >-  if (soname_regex_prop)
> >-    soname_regex = regex::compile(soname_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "soname_regexp", soname_regex);
> >
> >-  ini::simple_property_sptr soname_not_regex_prop =
> >-    is_simple_property(section.find_property("soname_not_regexp"));
> >   regex_t_sptr soname_not_regex;
> >-  if (soname_not_regex_prop)
> >-    soname_not_regex =
> >-      regex::compile(soname_not_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "soname_not_regexp", soname_not_regex);
> >
> >   string name;
> >   maybe_get_string_prop(section, "name", name);
> >
> >-  ini::simple_property_sptr name_regex_prop =
> >-    is_simple_property(section.find_property("name_regexp"));
> >   regex_t_sptr name_regex;
> >-  if (name_regex_prop)
> >-    name_regex = regex::compile(name_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "name_regexp", name_regex);
> >
> >-  ini::simple_property_sptr name_not_regex_prop =
> >-    is_simple_property(section.find_property("name_not_regexp"));
> >   regex_t_sptr name_not_regex;
> >-  if (name_not_regex_prop)
> >-    name_not_regex =
> >-      regex::compile(name_not_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "name_not_regexp", name_not_regex);
> >
> >   string return_type_name;
> >   maybe_get_string_prop(section, "return_type_name", return_type_name);
> >
> >-  ini::simple_property_sptr return_type_regex_prop =
> >-    is_simple_property(section.find_property("return_type_regexp"));
> >   regex_t_sptr return_type_regex;
> >-  if (return_type_regex_prop)
> >-    return_type_regex =
> >-      regex::compile(return_type_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "return_type_regexp", return_type_regex);
> >
> >   string sym_name;
> >   maybe_get_string_prop(section, "symbol_name", sym_name);
> >
> >-  ini::simple_property_sptr sym_name_regex_prop =
> >-    is_simple_property(section.find_property("symbol_name_regexp"));
> >   regex_t_sptr sym_name_regex;
> >-  if (sym_name_regex_prop)
> >-    sym_name_regex =
> >-      regex::compile(sym_name_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "symbol_name_regexp", sym_name_regex);
> >
> >-  ini::simple_property_sptr sym_name_not_regex_prop =
> >-    is_simple_property(section.find_property("symbol_name_not_regexp"));
> >   regex_t_sptr sym_name_not_regex;
> >-  if (sym_name_not_regex_prop)
> >-    sym_name_not_regex =
> >-      regex::compile(sym_name_not_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "symbol_name_not_regexp", sym_name_not_regex);
> >
> >   string sym_version;
> >   maybe_get_string_prop(section, "symbol_version", sym_version);
> >
> >-  ini::simple_property_sptr sym_ver_regex_prop =
> >-    is_simple_property(section.find_property("symbol_version_regexp"));
> >   regex_t_sptr sym_ver_regex;
> >-  if (sym_ver_regex_prop)
> >-    sym_ver_regex =
> >-      regex::compile(sym_ver_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "symbol_version_regexp", sym_ver_regex);
> >
> >   string allow_other_aliases;
> >   maybe_get_string_prop(section, "allow_other_aliases", allow_other_aliases);
> >@@ -4025,85 +3985,47 @@ read_variable_suppression(const ini::config::section& section)
> >   string label_str;
> >   maybe_get_string_prop(section, "label", label_str);
> >
> >-  ini::simple_property_sptr file_name_regex_prop =
> >-    is_simple_property(section.find_property("file_name_regexp"));
> >   regex_t_sptr file_name_regex;
> >-  if (file_name_regex_prop)
> >-    file_name_regex =
> >-      regex::compile(file_name_regex_prop->get_value()->as_string());
> >-
> >- ini::simple_property_sptr file_name_not_regex_prop =
> >-  is_simple_property(section.find_property("file_name_not_regexp"));
> >- regex_t_sptr file_name_not_regex;
> >- if (file_name_not_regex_prop)
> >-   file_name_not_regex =
> >-     regex::compile(file_name_not_regex_prop->get_value()->as_string());
> >-
> >-  ini::simple_property_sptr soname_regex_prop =
> >-    is_simple_property(section.find_property("soname_regexp"));
> >+  maybe_get_regex_prop(section, "file_name_regexp", file_name_regex);
> >+
> >+  regex_t_sptr file_name_not_regex;
> >+  maybe_get_regex_prop(section, "file_name_not_regexp", file_name_not_regex);
> >+
> >   regex_t_sptr soname_regex;
> >-  if (soname_regex_prop)
> >-    soname_regex = regex::compile(soname_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "soname_regexp", soname_regex);
> >
> >-  ini::simple_property_sptr soname_not_regex_prop =
> >-    is_simple_property(section.find_property("soname_not_regexp"));
> >   regex_t_sptr soname_not_regex;
> >-  if (soname_not_regex_prop)
> >-    soname_not_regex =
> >-      regex::compile(soname_not_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "soname_not_regexp", soname_not_regex);
> >
> >   string name_str;
> >   maybe_get_string_prop(section, "name", name_str);
> >
> >-  ini::simple_property_sptr name_regex_prop =
> >-    is_simple_property(section.find_property("name_regexp"));
> >   regex_t_sptr name_regex;
> >-  if (name_regex_prop)
> >-    name_regex = regex::compile(name_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "name_regexp", name_regex);
> >
> >-  ini::simple_property_sptr name_not_regex_prop =
> >-    is_simple_property(section.find_property("name_not_regexp"));
> >   regex_t_sptr name_not_regex;
> >-  if (name_not_regex_prop)
> >-    name_not_regex =
> >-      regex::compile(name_not_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "name_not_regexp", name_not_regex);
> >
> >   string symbol_name;
> >   maybe_get_string_prop(section, "symbol_name", symbol_name);
> >
> >-  ini::simple_property_sptr sym_name_regex_prop =
> >-    is_simple_property(section.find_property("symbol_name_regexp"));
> >   regex_t_sptr symbol_name_regex;
> >-  if (sym_name_regex_prop)
> >-    symbol_name_regex =
> >-      regex::compile(sym_name_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "symbol_name_regexp", symbol_name_regex);
> >
> >-  ini::simple_property_sptr sym_name_not_regex_prop =
> >-    is_simple_property(section.find_property("symbol_name_not_regexp"));
> >   regex_t_sptr symbol_name_not_regex;
> >-  if (sym_name_not_regex_prop)
> >-    symbol_name_not_regex =
> >-      regex::compile(sym_name_not_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "symbol_name_not_regexp", symbol_name_not_regex);
> >
> >   string symbol_version;
> >   maybe_get_string_prop(section, "symbol_version", symbol_version);
> >
> >-  ini::simple_property_sptr sym_version_regex_prop =
> >-    is_simple_property(section.find_property("symbol_version_regexp"));
> >   regex_t_sptr symbol_version_regex;
> >-  if (sym_version_regex_prop)
> >-    symbol_version_regex =
> >-      regex::compile(sym_version_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "symbol_version_regexp", symbol_version_regex);
> >
> >   string type_name_str;
> >   maybe_get_string_prop(section, "type_name", type_name_str);
> >
> >-  ini::simple_property_sptr type_name_regex_prop =
> >-    is_simple_property(section.find_property("type_name_regexp"));
> >   regex_t_sptr type_name_regex;
> >-  if (type_name_regex_prop)
> >-    type_name_regex =
> >-      regex::compile(type_name_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "type_name_regexp", type_name_regex);
> >
> >   result.reset(new variable_suppression(label_str,
> >                                       name_str,
> >@@ -4246,32 +4168,17 @@ read_file_suppression(const ini::config::section& section)
> >   string label_str;
> >   maybe_get_string_prop(section, "label", label_str);
> >
> >-  ini::simple_property_sptr file_name_regex_prop =
> >-    is_simple_property(section.find_property("file_name_regexp"));
> >   regex_t_sptr file_name_regex;
> >-  if (file_name_regex_prop)
> >-    file_name_regex =
> >-      regex::compile(file_name_regex_prop->get_value()->as_string());
> >-
> >- ini::simple_property_sptr file_name_not_regex_prop =
> >-    is_simple_property(section.find_property("file_name_not_regexp"));
> >- regex_t_sptr file_name_not_regex;
> >- if (file_name_not_regex_prop)
> >-   file_name_not_regex =
> >-     regex::compile(file_name_not_regex_prop->get_value()->as_string());
> >-
> >-  ini::simple_property_sptr soname_regex_prop =
> >-    is_simple_property(section.find_property("soname_regexp"));
> >+  maybe_get_regex_prop(section, "file_name_regexp", file_name_regex);
> >+
> >+  regex_t_sptr file_name_not_regex;
> >+  maybe_get_regex_prop(section, "file_name_not_regexp", file_name_not_regex);
> >+
> >   regex_t_sptr soname_regex;
> >-  if (soname_regex_prop)
> >-    soname_regex = regex::compile(soname_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "soname_regexp", soname_regex);
> >
> >-  ini::simple_property_sptr soname_not_regex_prop =
> >-    is_simple_property(section.find_property("soname_not_regexp"));
> >   regex_t_sptr soname_not_regex;
> >-  if (soname_not_regex_prop)
> >-    soname_not_regex =
> >-      regex::compile(soname_not_regex_prop->get_value()->as_string());
> >+  maybe_get_regex_prop(section, "soname_not_regexp", soname_not_regex);
> >
> >   result.reset(new file_suppression(label_str,
> >                                   file_name_regex,
> >--
> >2.26.2.303.gf8c07b1a785-goog
> >