[PATCH 0/7] Suppression parsing - preparatory work

[PATCH 0/7] Suppression parsing - preparatory work

[Giuliano Procida]

Hi Dodji.

Quite a while ago I had a series of patches with the aim of improving
libabigail's suppression parsing with the main aims:

* adding error handling and reporting
* refactoring for easier maintenance (both fixes and features)

Early on in the series, I changed the way regexes were parsed and
passed in and out of the suppression specifications. This wasn't
something you were happy with, so I shelved the series.

I've taken a lttle time to remove those changes and rebase the series.
My plan is to feed changes to you in digistible batches.

This batch contains:

* 2 commits that fix issues in an uncontroversial way
* 3 commits to add the outer shell of error handling
* 2 commits to simplify how suppressions are constructed

The error handling commits do not add any error reporting but do add
placeholder TODOs for where this could be added.

The constructor change commits remove the non-default constructors for
the 4 suppression types as they are antithetical to a table-driver
parser where there are a large number of optional fields. If these
constructors need to be preserved for other reasons, that can still be
done. For (human) use, a better interface for building suppressions
might be where setters can be chained (a.k.a. fluent interface) but
that might not be your preference.

Please take a fresh look at these.

If you wanted to look ahead to what the end state could be, please see
https://github.com/myxoid/libabigail/tree/suppression-parsing-revisited
but note that the last few commits are not ready for consumption.

Thank you,
Giuliano.

Giuliano Procida (7):
  Add missing newlines to end of test files.
  Fix two wrongs in test suppression regex
  Better suppression section parsing delegation.
  Add read_*_suppression success/failure plumbing.
  Add error handling to read_suppressions.
  Default construct suppression types.
  Refresh getter/setter comments.

 include/abg-suppression.h                     |  48 +-
 src/abg-suppression-priv.h                    |  28 +-
 src/abg-suppression.cc                        | 618 +++++++-----------
 src/abg-tools-utils.cc                        |   6 +-
 .../test-diff-suppr/test0-type-suppr-2.suppr  |   2 +-
 .../test22-suppr-removed-var-sym-4.suppr      |   2 +-
 .../test23-alias-filter-0.suppr               |   2 +-
 .../test23-alias-filter-4.suppr               |   2 +-
 .../test28-add-aliased-function-1.suppr       |   2 +-
 .../test28-add-aliased-function-2.suppr       |   2 +-
 .../test28-add-aliased-function-3.suppr       |   2 +-
 .../test28-add-aliased-function-4.suppr       |   2 +-
 .../test38-char-class-in-ini.abignore         |   2 +-
 .../test41-enumerator-changes-0.suppr         |   2 +-
 .../test-diff-suppr/test7-var-suppr-7.suppr   |   2 +-
 .../test01-equal-in-property-string.abignore  |   2 +-
 16 files changed, 274 insertions(+), 450 deletions(-)

-- 
2.28.0.220.ged08abb693-goog

[PATCH 1/7] Add missing newlines to end of test files.

[Giuliano Procida]

Various test files were missing terminal newlines.

	* tests/data/test-diff-suppr/test0-type-suppr-2.suppr: Add
	final new line.
	* tests/data/test-diff-suppr/test22-suppr-removed-var-sym-4.suppr:
	Likewise.
	* tests/data/test-diff-suppr/test23-alias-filter-0.suppr:
	Likewise.
	* tests/data/test-diff-suppr/test23-alias-filter-4.suppr:
	Likewise.
	* tests/data/test-diff-suppr/test28-add-aliased-function-1.suppr:
	Likewise.
	* tests/data/test-diff-suppr/test28-add-aliased-function-2.suppr:
	Likewise.
	* tests/data/test-diff-suppr/test28-add-aliased-function-3.suppr:
	Likewise.
	* tests/data/test-diff-suppr/test28-add-aliased-function-4.suppr:
	Likewise.
	* tests/data/test-diff-suppr/test41-enumerator-changes-0.suppr:
	Likewise.
	* tests/data/test-diff-suppr/test7-var-suppr-7.suppr:
	Likewise.
	* tests/data/test-ini/test01-equal-in-property-string.abignore:
	Likewise.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 tests/data/test-diff-suppr/test0-type-suppr-2.suppr             | 2 +-
 tests/data/test-diff-suppr/test22-suppr-removed-var-sym-4.suppr | 2 +-
 tests/data/test-diff-suppr/test23-alias-filter-0.suppr          | 2 +-
 tests/data/test-diff-suppr/test23-alias-filter-4.suppr          | 2 +-
 tests/data/test-diff-suppr/test28-add-aliased-function-1.suppr  | 2 +-
 tests/data/test-diff-suppr/test28-add-aliased-function-2.suppr  | 2 +-
 tests/data/test-diff-suppr/test28-add-aliased-function-3.suppr  | 2 +-
 tests/data/test-diff-suppr/test28-add-aliased-function-4.suppr  | 2 +-
 tests/data/test-diff-suppr/test41-enumerator-changes-0.suppr    | 2 +-
 tests/data/test-diff-suppr/test7-var-suppr-7.suppr              | 2 +-
 tests/data/test-ini/test01-equal-in-property-string.abignore    | 2 +-
 11 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/tests/data/test-diff-suppr/test0-type-suppr-2.suppr b/tests/data/test-diff-suppr/test0-type-suppr-2.suppr
index 0b9d00f57..9b9ed74c5 100644
--- a/tests/data/test-diff-suppr/test0-type-suppr-2.suppr
+++ b/tests/data/test-diff-suppr/test0-type-suppr-2.suppr
@@ -1,2 +1,2 @@
 [suppress_type]
-  name = MyType::private
\ No newline at end of file
+  name = MyType::private
diff --git a/tests/data/test-diff-suppr/test22-suppr-removed-var-sym-4.suppr b/tests/data/test-diff-suppr/test22-suppr-removed-var-sym-4.suppr
index 79252b396..545ab8f69 100644
--- a/tests/data/test-diff-suppr/test22-suppr-removed-var-sym-4.suppr
+++ b/tests/data/test-diff-suppr/test22-suppr-removed-var-sym-4.suppr
@@ -2,4 +2,4 @@
   symbol_name = global_var1
 
 [suppress_variable]
-  symbol_name = global_var2
\ No newline at end of file
+  symbol_name = global_var2
diff --git a/tests/data/test-diff-suppr/test23-alias-filter-0.suppr b/tests/data/test-diff-suppr/test23-alias-filter-0.suppr
index 40e47ef00..cd752c08c 100644
--- a/tests/data/test-diff-suppr/test23-alias-filter-0.suppr
+++ b/tests/data/test-diff-suppr/test23-alias-filter-0.suppr
@@ -1,3 +1,3 @@
 [suppress_function]
   name_regexp = ^__private_.*|^function.
-  allow_other_aliases = yes
\ No newline at end of file
+  allow_other_aliases = yes
diff --git a/tests/data/test-diff-suppr/test23-alias-filter-4.suppr b/tests/data/test-diff-suppr/test23-alias-filter-4.suppr
index 2a7e5893e..b7429d145 100644
--- a/tests/data/test-diff-suppr/test23-alias-filter-4.suppr
+++ b/tests/data/test-diff-suppr/test23-alias-filter-4.suppr
@@ -1,3 +1,3 @@
  [suppress_function]
    symbol_name_regexp = function1
-   allow_other_aliases = no
\ No newline at end of file
+   allow_other_aliases = no
diff --git a/tests/data/test-diff-suppr/test28-add-aliased-function-1.suppr b/tests/data/test-diff-suppr/test28-add-aliased-function-1.suppr
index 797c33645..47508baeb 100644
--- a/tests/data/test-diff-suppr/test28-add-aliased-function-1.suppr
+++ b/tests/data/test-diff-suppr/test28-add-aliased-function-1.suppr
@@ -1,3 +1,3 @@
 [suppress_function]
     name_regexp = bar
-    change_kind = added-function
\ No newline at end of file
+    change_kind = added-function
diff --git a/tests/data/test-diff-suppr/test28-add-aliased-function-2.suppr b/tests/data/test-diff-suppr/test28-add-aliased-function-2.suppr
index 58243db76..f2075a247 100644
--- a/tests/data/test-diff-suppr/test28-add-aliased-function-2.suppr
+++ b/tests/data/test-diff-suppr/test28-add-aliased-function-2.suppr
@@ -1,3 +1,3 @@
 [suppress_function]
     name_regexp = bar|baz
-    change_kind = added-function
\ No newline at end of file
+    change_kind = added-function
diff --git a/tests/data/test-diff-suppr/test28-add-aliased-function-3.suppr b/tests/data/test-diff-suppr/test28-add-aliased-function-3.suppr
index 7e44b2c0d..3b3b51893 100644
--- a/tests/data/test-diff-suppr/test28-add-aliased-function-3.suppr
+++ b/tests/data/test-diff-suppr/test28-add-aliased-function-3.suppr
@@ -1,3 +1,3 @@
 [suppress_function]
     symbol_name = bar
-    change_kind = added-function
\ No newline at end of file
+    change_kind = added-function
diff --git a/tests/data/test-diff-suppr/test28-add-aliased-function-4.suppr b/tests/data/test-diff-suppr/test28-add-aliased-function-4.suppr
index 3e50224d1..1acf616af 100644
--- a/tests/data/test-diff-suppr/test28-add-aliased-function-4.suppr
+++ b/tests/data/test-diff-suppr/test28-add-aliased-function-4.suppr
@@ -1,3 +1,3 @@
 [suppress_function]
     symbol_name_regexp = bar
-    change_kind = added-function
\ No newline at end of file
+    change_kind = added-function
diff --git a/tests/data/test-diff-suppr/test41-enumerator-changes-0.suppr b/tests/data/test-diff-suppr/test41-enumerator-changes-0.suppr
index e6bdf779a..7f98af418 100644
--- a/tests/data/test-diff-suppr/test41-enumerator-changes-0.suppr
+++ b/tests/data/test-diff-suppr/test41-enumerator-changes-0.suppr
@@ -1,4 +1,4 @@
 [suppress_type]
  type_kind = enum
  name = EnumType1
- changed_enumerators = enum_count
\ No newline at end of file
+ changed_enumerators = enum_count
diff --git a/tests/data/test-diff-suppr/test7-var-suppr-7.suppr b/tests/data/test-diff-suppr/test7-var-suppr-7.suppr
index 70bf1d379..3c5844761 100644
--- a/tests/data/test-diff-suppr/test7-var-suppr-7.suppr
+++ b/tests/data/test-diff-suppr/test7-var-suppr-7.suppr
@@ -1,3 +1,3 @@
 [suppress_variable]
   name = var1
-  type_name = S1*
\ No newline at end of file
+  type_name = S1*
diff --git a/tests/data/test-ini/test01-equal-in-property-string.abignore b/tests/data/test-ini/test01-equal-in-property-string.abignore
index 8c70b460a..ffeee66bb 100644
--- a/tests/data/test-ini/test01-equal-in-property-string.abignore
+++ b/tests/data/test-ini/test01-equal-in-property-string.abignore
@@ -1,4 +1,4 @@
 [suppress_file]
   # Somme comment
   label = Libabigail can't handle libgfortran.so (https://sourceware.org/bugzilla/show_bug.cgi?id=23492)
-  file_name_regexp = libgfortran\\.so.*
\ No newline at end of file
+  file_name_regexp = libgfortran\\.so.*
-- 
2.28.0.220.ged08abb693-goog

[PATCH 2/7] Fix two wrongs in test suppression regex

[Giuliano Procida]

The suppression specification in test38-char-class-in-ini.abignore was
introduced in commit 1478d9cc1c74ca3a2be89cd09aac5afcbdf818c7.

Unfortunately it contains two errors. One causes the file name not to
match as the string is the full path, not the base name. The other is
a typo that causes the file name match not to even be attempted. The
two mistakes cancel in the test, but result in a suppression
specification that is broader than intended.

	* tests/data/test-diff-suppr/test38-char-class-in-ini.abignore:
	Don't anchor regex match to beginning of file name.
	Change "filename_regexp" to "file_name_regexp".

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 tests/data/test-diff-suppr/test38-char-class-in-ini.abignore | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/data/test-diff-suppr/test38-char-class-in-ini.abignore b/tests/data/test-diff-suppr/test38-char-class-in-ini.abignore
index 07f3f91f8..e96240cd4 100644
--- a/tests/data/test-diff-suppr/test38-char-class-in-ini.abignore
+++ b/tests/data/test-diff-suppr/test38-char-class-in-ini.abignore
@@ -1,4 +1,4 @@
 [suppress_function]
- filename_regexp = ^test38-char-class-in-ini-v[[:digit:]].*
+ file_name_regexp = test38-char-class-in-ini-v[[:digit:]].*
  symbol_name_regexp = bar
  change_kind = added-function
-- 
2.28.0.220.ged08abb693-goog

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

[Giuliano Procida]

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(-)

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);
-
+    {
+      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",
-- 
2.28.0.220.ged08abb693-goog

[PATCH 4/7] Add read_*_suppression success/failure plumbing.

[Giuliano Procida]

In order to track parse success/failure, we need all parsing functions
to emit this as well any parsed values. The simplest way to do this is
to return parse success and to assign parsed values to output
parameters. This is a pattern that is already present in the code
base.

This patch does this for the four main parsing functions.

	* src/abg-suppression.cc (read_type_suppression): Return
	result via assignment to reference argument and return true
	iff input section was parsed successfully.
	(read_function_suppression): Ditto.
	(read_variable_suppression): Ditto.
	(read_file_suppression): Ditto.

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

diff --git a/src/abg-suppression.cc b/src/abg-suppression.cc
index 682bb8742..736c7f51f 100644
--- a/src/abg-suppression.cc
+++ b/src/abg-suppression.cc
@@ -346,17 +346,21 @@ names_of_binaries_match(const suppression_base& suppr,
 suppression_base::~suppression_base()
 {}
 
-static type_suppression_sptr
-read_type_suppression(const ini::config::section& section);
+static bool
+read_type_suppression(const ini::config::section& section,
+		      suppression_sptr& suppr);
 
-static function_suppression_sptr
-read_function_suppression(const ini::config::section& section);
+static bool
+read_function_suppression(const ini::config::section& section,
+			  suppression_sptr& suppr);
 
-static variable_suppression_sptr
-read_variable_suppression(const ini::config::section& section);
+static bool
+read_variable_suppression(const ini::config::section& section,
+			  suppression_sptr& suppr);
 
-static file_suppression_sptr
-read_file_suppression(const ini::config::section& section);
+static bool
+read_file_suppression(const ini::config::section& section,
+		      suppression_sptr& suppr);
 
 /// Read a vector of suppression specifications from the sections of
 /// an ini::config.
@@ -381,13 +385,13 @@ read_suppressions(const ini::config& config,
       const std::string& name = section->get_name();
       suppression_sptr s;
       if (name == "suppress_type")
-	s = read_type_suppression(*section);
+	read_type_suppression(*section, s);
       else if (name == "suppress_function")
-	s = read_function_suppression(*section);
+	read_function_suppression(*section, s);
       else if (name == "suppress_variable")
-	s = read_variable_suppression(*section);
+	read_variable_suppression(*section, s);
       else if (name == "suppress_file")
-	s = read_file_suppression(*section);
+	read_file_suppression(*section, s);
       if (s)
 	suppressions.push_back(s);
     }
@@ -1565,13 +1569,13 @@ read_suppression_reach_kind(const string& input)
 ///
 /// @param section the section of the ini config to read.
 ///
-/// @return the resulting @ref type_suppression upon successful
-/// parsing, or nil.
-static type_suppression_sptr
-read_type_suppression(const ini::config::section& section)
+/// @param suppr the @ref suppression to assign to.
+///
+/// @return whether the parse was successful.
+static bool
+read_type_suppression(const ini::config::section& section,
+		      suppression_sptr& suppr)
 {
-  type_suppression_sptr result;
-
   static const char *const sufficient_props[] = {
     "file_name_regexp",
     "file_name_not_regexp",
@@ -1587,7 +1591,7 @@ read_type_suppression(const ini::config::section& section)
   if (!check_sufficient_props(sufficient_props,
 			      sizeof(sufficient_props)/sizeof(char*),
 			      section))
-    return result;
+    return false;
 
   ini::simple_property_sptr drop_artifact =
     is_simple_property(section.find_property("drop_artifact"));
@@ -1711,7 +1715,7 @@ read_type_suppression(const ini::config::section& section)
 	       type_suppression::insertion_range::create_fn_call_expr_boundary(ini::read_function_call_expr(ins_point)))
 	begin = expr;
       else
-	return result;
+	return false;
 
       end = type_suppression::insertion_range::create_integer_boundary(-1);
       type_suppression::insertion_range_sptr insert_range
@@ -1754,7 +1758,7 @@ read_type_suppression(const ini::config::section& section)
 		   type_suppression::insertion_range::create_fn_call_expr_boundary(ini::read_function_call_expr(str)))
 	    begin = expr;
 	  else
-	    return result;
+	    return false;
 
 	  str = val->get_content()[1];
 	  if (str == "end")
@@ -1767,7 +1771,7 @@ read_type_suppression(const ini::config::section& section)
 		   type_suppression::insertion_range::create_fn_call_expr_boundary(ini::read_function_call_expr(str)))
 	    end = expr;
 	  else
-	    return result;
+	    return false;
 
 	  type_suppression::insertion_range_sptr insert_range
 	    (new type_suppression::insertion_range(begin, end));
@@ -1778,7 +1782,7 @@ read_type_suppression(const ini::config::section& section)
 	// the 'has_data_member_inserted_between' property has a wrong
 	// value type, so let's discard the endire [suppress_type]
 	// section.
-	return result;
+	return false;
     }
 
   // Support has_data_members_inserted_between
@@ -1829,7 +1833,7 @@ read_type_suppression(const ini::config::section& section)
 		   type_suppression::insertion_range::create_fn_call_expr_boundary(ini::read_function_call_expr(str)))
 	    begin = expr;
 	  else
-	    return result;
+	    return false;
 
 	  str = list_value->get_content()[1];
 	  if (str == "end")
@@ -1842,7 +1846,7 @@ read_type_suppression(const ini::config::section& section)
 		   type_suppression::insertion_range::create_fn_call_expr_boundary(ini::read_function_call_expr(str)))
 	    end = expr;
 	  else
-	    return result;
+	    return false;
 
 	  type_suppression::insertion_range_sptr insert_range
 	    (new type_suppression::insertion_range(begin, end));
@@ -1850,7 +1854,7 @@ read_type_suppression(const ini::config::section& section)
 	  consider_data_member_insertion = true;
 	}
       if (!is_well_formed)
-	return result;
+	return false;
     }
 
   /// Support 'changed_enumerators = foo, bar, baz'
@@ -1877,56 +1881,57 @@ read_type_suppression(const ini::config::section& section)
 	changed_enumerator_names.push_back(p->get_value()->as_string());
     }
 
-  result.reset(new type_suppression(label_str, name_regex_str, name_str));
+  type_suppression result(label_str, name_regex_str, name_str);
 
   if (consider_type_kind)
     {
-      result->set_consider_type_kind(true);
-      result->set_type_kind(type_kind);
+      result.set_consider_type_kind(true);
+      result.set_type_kind(type_kind);
     }
 
   if (consider_reach_kind)
     {
-      result->set_consider_reach_kind(true);
-      result->set_reach_kind(reach_kind);
+      result.set_consider_reach_kind(true);
+      result.set_reach_kind(reach_kind);
     }
 
   if (consider_data_member_insertion)
-    result->set_data_member_insertion_ranges(insert_ranges);
+    result.set_data_member_insertion_ranges(insert_ranges);
 
   if (!name_not_regex_str.empty())
-    result->set_type_name_not_regex_str(name_not_regex_str);
+    result.set_type_name_not_regex_str(name_not_regex_str);
 
   if (!file_name_regex_str.empty())
-    result->set_file_name_regex_str(file_name_regex_str);
+    result.set_file_name_regex_str(file_name_regex_str);
 
   if (!file_name_not_regex_str.empty())
-    result->set_file_name_not_regex_str(file_name_not_regex_str);
+    result.set_file_name_not_regex_str(file_name_not_regex_str);
 
   if (!soname_regex_str.empty())
-    result->set_soname_regex_str(soname_regex_str);
+    result.set_soname_regex_str(soname_regex_str);
 
   if (!soname_not_regex_str.empty())
-    result->set_soname_not_regex_str(soname_not_regex_str);
+    result.set_soname_not_regex_str(soname_not_regex_str);
 
   if (!srcloc_not_in.empty())
-    result->set_source_locations_to_keep(srcloc_not_in);
+    result.set_source_locations_to_keep(srcloc_not_in);
 
   if (!srcloc_not_regexp_str.empty())
-    result->set_source_location_to_keep_regex_str(srcloc_not_regexp_str);
+    result.set_source_location_to_keep_regex_str(srcloc_not_regexp_str);
 
   if ((drop_artifact_str == "yes" || drop_artifact_str == "true")
       && ((!name_regex_str.empty()
 	   || !name_str.empty()
 	   || !srcloc_not_regexp_str.empty()
 	   || !srcloc_not_in.empty())))
-    result->set_drops_artifact_from_ir(true);
+    result.set_drops_artifact_from_ir(true);
 
-  if (result->get_type_kind() == type_suppression::ENUM_TYPE_KIND
+  if (result.get_type_kind() == type_suppression::ENUM_TYPE_KIND
       && !changed_enumerator_names.empty())
-    result->set_changed_enumerator_names(changed_enumerator_names);
+    result.set_changed_enumerator_names(changed_enumerator_names);
 
-  return result;
+  suppr.reset(new type_suppression(result));
+  return true;
 }
 
 // <function_suppression stuff>
@@ -3146,18 +3151,19 @@ read_parameter_spec_from_string(const string& str)
   return result;
 }
 
-/// Parse function suppression specification, build a resulting @ref
-/// function_suppression type and return a shared pointer to that
-/// object.
+/// Read a function suppression from an instance of
+/// ini::config::section and build a @ref function_suppression as a
+/// result.
+///
+/// @param section the section of the ini config to read.
 ///
-/// @return a shared pointer to the newly built @ref
-/// function_suppression.  If the function suppression specification
-/// could not be parsed then a nil shared pointer is returned.
-static function_suppression_sptr
-read_function_suppression(const ini::config::section& section)
+/// @param suppr the @ref suppression to assign to.
+///
+/// @return whether the parse was successful.
+static bool
+read_function_suppression(const ini::config::section& section,
+			  suppression_sptr& suppr)
 {
-  function_suppression_sptr result;
-
   static const char *const sufficient_props[] = {
     "label",
     "file_name_regexp",
@@ -3179,7 +3185,7 @@ read_function_suppression(const ini::config::section& section)
   if (!check_sufficient_props(sufficient_props,
 			      sizeof(sufficient_props)/sizeof(char*),
 			      section))
-    return result;
+    return false;
 
   ini::simple_property_sptr drop_artifact =
     is_simple_property(section.find_property("drop_artifact"));
@@ -3307,16 +3313,16 @@ read_function_suppression(const ini::config::section& section)
 	  parms.push_back(parm);
       }
 
-  result.reset(new function_suppression(label_str,
-					name,
-					name_regex_str,
-					return_type_name,
-					return_type_regex_str,
-					parms,
-					sym_name,
-					sym_name_regex_str,
-					sym_version,
-					sym_ver_regex_str));
+  function_suppression result(label_str,
+			      name,
+			      name_regex_str,
+			      return_type_name,
+			      return_type_regex_str,
+			      parms,
+			      sym_name,
+			      sym_name_regex_str,
+			      sym_version,
+			      sym_ver_regex_str);
 
   if ((drop_artifact_str == "yes" || drop_artifact_str == "true")
       && (!name.empty()
@@ -3325,35 +3331,36 @@ read_function_suppression(const ini::config::section& section)
 	  || !sym_name.empty()
 	  || !sym_name_regex_str.empty()
 	  || !sym_name_not_regex_str.empty()))
-    result->set_drops_artifact_from_ir(true);
+    result.set_drops_artifact_from_ir(true);
 
   if (!change_kind_str.empty())
-    result->set_change_kind
+    result.set_change_kind
       (function_suppression::parse_change_kind(change_kind_str));
 
   if (!allow_other_aliases.empty())
-    result->set_allow_other_aliases(allow_other_aliases == "yes"
-				    || allow_other_aliases == "true");
+    result.set_allow_other_aliases(allow_other_aliases == "yes"
+				   || allow_other_aliases == "true");
 
   if (!name_not_regex_str.empty())
-    result->set_name_not_regex_str(name_not_regex_str);
+    result.set_name_not_regex_str(name_not_regex_str);
 
   if (!sym_name_not_regex_str.empty())
-    result->set_symbol_name_not_regex_str(sym_name_not_regex_str);
+    result.set_symbol_name_not_regex_str(sym_name_not_regex_str);
 
   if (!file_name_regex_str.empty())
-    result->set_file_name_regex_str(file_name_regex_str);
+    result.set_file_name_regex_str(file_name_regex_str);
 
   if (!file_name_not_regex_str.empty())
-    result->set_file_name_not_regex_str(file_name_not_regex_str);
+    result.set_file_name_not_regex_str(file_name_not_regex_str);
 
   if (!soname_regex_str.empty())
-    result->set_soname_regex_str(soname_regex_str);
+    result.set_soname_regex_str(soname_regex_str);
 
   if (!soname_not_regex_str.empty())
-    result->set_soname_not_regex_str(soname_not_regex_str);
+    result.set_soname_not_regex_str(soname_not_regex_str);
 
-  return result;
+  suppr.reset(new function_suppression(result));
+  return true;
 }
 
 // </function_suppression stuff>
@@ -4023,18 +4030,19 @@ operator|(variable_suppression::change_kind l,
     (static_cast<unsigned>(l) | static_cast<unsigned>(r));
 }
 
-/// Parse variable suppression specification, build a resulting @ref
-/// variable_suppression type and return a shared pointer to that
-/// object.
+/// Read a variable suppression from an instance of
+/// ini::config::section and build a @ref variable_suppression as a
+/// result.
+///
+/// @param section the section of the ini config to read.
+///
+/// @param suppr the @ref suppression to assign to.
 ///
-/// @return a shared pointer to the newly built @ref
-/// variable_suppression.  If the variable suppression specification
-/// could not be parsed then a nil shared pointer is returned.
-static variable_suppression_sptr
-read_variable_suppression(const ini::config::section& section)
+/// @return whether the parse was successful.
+static bool
+read_variable_suppression(const ini::config::section& section,
+			  suppression_sptr& suppr)
 {
-  variable_suppression_sptr result;
-
   static const char *const sufficient_props[] = {
     "label",
     "file_name_regexp",
@@ -4055,7 +4063,7 @@ read_variable_suppression(const ini::config::section& section)
   if (!check_sufficient_props(sufficient_props,
 			      sizeof(sufficient_props)/sizeof(char*),
 			      section))
-    return result;
+    return false;
 
   ini::simple_property_sptr drop_artifact =
     is_simple_property(section.find_property("drop_artifact"));
@@ -4162,15 +4170,15 @@ read_variable_suppression(const ini::config::section& section)
     ? type_name_regex_prop->get_value()->as_string()
      : "";
 
-  result.reset(new variable_suppression(label_str,
-					name_str,
-					name_regex_str,
-					symbol_name,
-					symbol_name_regex_str,
-					symbol_version,
-					symbol_version_regex_str,
-					type_name_str,
-					type_name_regex_str));
+  variable_suppression result(label_str,
+			      name_str,
+			      name_regex_str,
+			      symbol_name,
+			      symbol_name_regex_str,
+			      symbol_version,
+			      symbol_version_regex_str,
+			      type_name_str,
+			      type_name_regex_str);
 
   if ((drop_artifact_str == "yes" || drop_artifact_str == "true")
       && (!name_str.empty()
@@ -4179,31 +4187,32 @@ read_variable_suppression(const ini::config::section& section)
 	  || !symbol_name.empty()
 	  || !symbol_name_regex_str.empty()
 	  || !symbol_name_not_regex_str.empty()))
-    result->set_drops_artifact_from_ir(true);
+    result.set_drops_artifact_from_ir(true);
 
   if (!name_not_regex_str.empty())
-    result->set_name_not_regex_str(name_not_regex_str);
+    result.set_name_not_regex_str(name_not_regex_str);
 
   if (!symbol_name_not_regex_str.empty())
-    result->set_symbol_name_not_regex_str(symbol_name_not_regex_str);
+    result.set_symbol_name_not_regex_str(symbol_name_not_regex_str);
 
   if (!change_kind_str.empty())
-    result->set_change_kind
+    result.set_change_kind
       (variable_suppression::parse_change_kind(change_kind_str));
 
   if (!file_name_regex_str.empty())
-    result->set_file_name_regex_str(file_name_regex_str);
+    result.set_file_name_regex_str(file_name_regex_str);
 
   if (!file_name_not_regex_str.empty())
-    result->set_file_name_not_regex_str(file_name_not_regex_str);
+    result.set_file_name_not_regex_str(file_name_not_regex_str);
 
   if (!soname_regex_str.empty())
-    result->set_soname_regex_str(soname_regex_str);
+    result.set_soname_regex_str(soname_regex_str);
 
   if (!soname_not_regex_str.empty())
-    result->set_soname_not_regex_str(soname_not_regex_str);
+    result.set_soname_not_regex_str(soname_not_regex_str);
 
-  return result;
+  suppr.reset(new variable_suppression(result));
+  return true;
 }
 
 // </variable_suppression stuff>
@@ -4286,17 +4295,17 @@ file_suppression::~file_suppression()
 }
 
 /// Read a file suppression from an instance of ini::config::section
-/// and build a @ref type_suppression as a result.
+/// and build a @ref file_suppression as a result.
+///
+/// @param section the section of the ini config to read.
 ///
-/// @param section the section (from an ini file) to read the file
-/// suppression from.
+/// @param suppr the @ref suppression to assign to.
 ///
-/// @return file_suppression_sptr.
-static file_suppression_sptr
-read_file_suppression(const ini::config::section& section)
+/// @return whether the parse was successful.
+static bool
+read_file_suppression(const ini::config::section& section,
+		      suppression_sptr& suppr)
 {
-  file_suppression_sptr result;
-
   static const char *const sufficient_props[] = {
     "file_name_regexp",
     "file_name_not_regexp",
@@ -4306,7 +4315,7 @@ read_file_suppression(const ini::config::section& section)
   if (!check_sufficient_props(sufficient_props,
 			      sizeof(sufficient_props)/sizeof(char*),
 			      section))
-    return result;
+    return false;
 
   ini::simple_property_sptr label_prop =
     is_simple_property(section.find_property("label"));
@@ -4338,23 +4347,22 @@ read_file_suppression(const ini::config::section& section)
     ? soname_not_regex_prop->get_value()->as_string()
     : "";
 
-  result.reset(new file_suppression(label_str,
-				    file_name_regex_str,
-				    file_name_not_regex_str));
+  file_suppression result(label_str, file_name_regex_str, file_name_not_regex_str);
 
   if (!soname_regex_str.empty())
     {
-      result->set_soname_regex_str(soname_regex_str);
-      result->set_drops_artifact_from_ir(true);
+      result.set_soname_regex_str(soname_regex_str);
+      result.set_drops_artifact_from_ir(true);
     }
 
   if (!soname_not_regex_str.empty())
     {
-      result->set_soname_not_regex_str(soname_not_regex_str);
-      result->set_drops_artifact_from_ir(true);
+      result.set_soname_not_regex_str(soname_not_regex_str);
+      result.set_drops_artifact_from_ir(true);
     }
 
-  return result;
+  suppr.reset(new file_suppression(result));
+  return true;
 }
 
 /// Test if a given suppression specification is a file suppression
-- 
2.28.0.220.ged08abb693-goog

[PATCH 5/7] Add error handling to read_suppressions.

[Giuliano Procida]

The top-level parsing functions read_suppresions (one file-scoped
overload and two externally-visible overloads) have no error handling.
This change makes each return a success status and adds placeholders
for where error messages might be emitted.

There are no behavioural changes.

	* include/abg-suppression.h (read_suppresions): Change return
	type to bool.
	* src/abg-suppression.cc (read_suppresions): In the static
	worker overload, return parse success status. In the wrapper
	overloads return false if ini config parsing fails.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 include/abg-suppression.h |  4 +--
 src/abg-suppression.cc    | 57 +++++++++++++++++++++++++++++----------
 2 files changed, 45 insertions(+), 16 deletions(-)

diff --git a/include/abg-suppression.h b/include/abg-suppression.h
index 6383b9322..dbb52de35 100644
--- a/include/abg-suppression.h
+++ b/include/abg-suppression.h
@@ -130,11 +130,11 @@ public:
 					 const suppression_base& suppr);
 }; // end class suppression_base
 
-void
+bool
 read_suppressions(std::istream& input,
 		  suppressions_type& suppressions);
 
-void
+bool
 read_suppressions(const string& file_path,
 		  suppressions_type& suppressions);
 
diff --git a/src/abg-suppression.cc b/src/abg-suppression.cc
index 736c7f51f..fb97a124b 100644
--- a/src/abg-suppression.cc
+++ b/src/abg-suppression.cc
@@ -372,10 +372,12 @@ read_file_suppression(const ini::config::section& section,
 ///
 /// @param suppressions out parameter.  The vector of suppressions to
 /// append the newly read suppressions to.
-static void
-read_suppressions(const ini::config& config,
-		  suppressions_type& suppressions)
+///
+/// @return whether the parse was successful.
+static bool
+read_suppressions(const ini::config& config, suppressions_type& suppressions)
 {
+  bool success = true;
   for (ini::config::sections_type::const_iterator i =
 	 config.get_sections().begin();
        i != config.get_sections().end();
@@ -383,18 +385,31 @@ read_suppressions(const ini::config& config,
     {
       const ini::config::section_sptr& section = *i;
       const std::string& name = section->get_name();
+      bool section_success;
       suppression_sptr s;
       if (name == "suppress_type")
-	read_type_suppression(*section, s);
+	section_success = read_type_suppression(*section, s);
       else if (name == "suppress_function")
-	read_function_suppression(*section, s);
+	section_success = read_function_suppression(*section, s);
       else if (name == "suppress_variable")
-	read_variable_suppression(*section, s);
+	section_success = read_variable_suppression(*section, s);
       else if (name == "suppress_file")
-	read_file_suppression(*section, s);
-      if (s)
+	section_success = read_file_suppression(*section, s);
+      else
+	{
+	  // TODO: maybe emit unknown section name error
+	  success = false;
+	  continue;
+	}
+      if (section_success)
 	suppressions.push_back(s);
+      else
+	{
+	  // TODO: maybe emit section parse failure message
+	  success = false;
+	}
     }
+  return success;
 }
 
 /// Read suppressions specifications from an input stream.
@@ -403,12 +418,19 @@ read_suppressions(const ini::config& config,
 ///
 /// @param suppressions the vector of suppressions to append the newly
 /// read suppressions to.
-void
+///
+/// @return whether the parse was successful
+bool
 read_suppressions(std::istream& input,
 		  suppressions_type& suppressions)
 {
-    if (ini::config_sptr config = ini::read_config(input))
-    read_suppressions(*config, suppressions);
+  ini::config_sptr config = ini::read_config(input);
+  if (!config)
+    {
+      // TODO: maybe report ini configuration parse failure
+      return false;
+    }
+  return read_suppressions(*config, suppressions);
 }
 
 /// Read suppressions specifications from an input file on disk.
@@ -417,12 +439,19 @@ read_suppressions(std::istream& input,
 ///
 /// @param suppressions the vector of suppressions to append the newly
 /// read suppressions to.
-void
+///
+/// @return whether the parse was successful
+bool
 read_suppressions(const string& file_path,
 		  suppressions_type& suppressions)
 {
-  if (ini::config_sptr config = ini::read_config(file_path))
-    read_suppressions(*config, suppressions);
+  ini::config_sptr config = ini::read_config(file_path);
+  if (!config)
+    {
+      // TODO: maybe report ini configuration file_path parse failure
+      return false;
+    }
+  return read_suppressions(*config, suppressions);
 }
 // </suppression_base stuff>
 
-- 
2.28.0.220.ged08abb693-goog

[PATCH 6/7] Default construct suppression types.

[Giuliano Procida]

The current constructors for the various suppression types are not a
good match for their current uses. Given that every member is
effectively optional, default constructing these to a default
unpopulated state makes the code simpler.

There are no behavioural changes.

	* include/abg-suppression.h (suppression_base): Remove both
	non-default constructors, make default constructor public.
	(type_suppression): Ditto.  (function_suppression): Remove
	non-default constructor.  (variable_suppression): Remove
	non-default constructor, add default constructor.
	(file_suppression): Remove non-default constructor, make
	default constructor public.
	* src/abg-suppression-priv.h (variable_suppression::priv):
	Replace constructor with default constructor.
	(type_suppression::priv): Make default constructor public and
	define it.
	* src/abg-suppression.cc (suppression_base): Drop non-default
	constructors, define default constructor.
	(type_suppression): Drop non-default constructor, define
	default constructor. (function_suppression): Drop non-default
	constructor.  (variable_suppression): Drop non-default
	constructor, define default constructor.  (file_suppression):
	Drop non-default constructor, define default constructor.
	(read_type_suppression): Default construct result and populate
	initial fields using setters.  (read_function_suppression):
	Ditto.	(read_function_suppression): Ditto.
	(read_file_suppression): Ditto.
	* src/abg-tools-utils.cc (handle_file_entry): Default
	construct type_suppression suppr and use set_label to set
	initial field.

Reviewed-by: Matthias Maennich <maennich@google.com>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 include/abg-suppression.h  |  44 +-----
 src/abg-suppression-priv.h |  28 ++--
 src/abg-suppression.cc     | 270 ++++++-------------------------------
 src/abg-tools-utils.cc     |   6 +-
 4 files changed, 59 insertions(+), 289 deletions(-)

diff --git a/include/abg-suppression.h b/include/abg-suppression.h
index dbb52de35..d1246a405 100644
--- a/include/abg-suppression.h
+++ b/include/abg-suppression.h
@@ -55,17 +55,10 @@ class suppression_base
   class priv;
   typedef shared_ptr<priv> priv_sptr;
 
-  // Forbid default constructor
-  suppression_base();
-
 public:
   priv_sptr priv_;
 
-  suppression_base(const string& label);
-
-  suppression_base(const string& label,
-		   const string& file_name_regex_str,
-		   const string& file_name_not_regex_str);
+  suppression_base();
 
   bool
   get_drops_artifact_from_ir() const;
@@ -155,9 +148,6 @@ class type_suppression : public suppression_base
   class priv;
   typedef shared_ptr<priv> priv_sptr;
 
-  // Forbid this;
-  type_suppression();
-
 public:
 
   priv_sptr priv_;
@@ -203,9 +193,7 @@ public:
   /// A convenience typedef for a vector of @ref insertion_range_sptr.
   typedef vector<insertion_range_sptr> insertion_ranges;
 
-  type_suppression(const string& label,
-		   const string& type_name_regexp,
-		   const string& type_name);
+  type_suppression();
 
   virtual ~type_suppression();
 
@@ -460,17 +448,6 @@ public:
 
   function_suppression();
 
-  function_suppression(const string&		label,
-		       const string&		name,
-		       const string&		name_regex,
-		       const string&		return_type_name,
-		       const string&		return_type_regex,
-		       parameter_specs_type&	parm_specs,
-		       const string&		symbol_name,
-		       const string&		symbol_name_regex,
-		       const string&		symbol_version,
-		       const string&		symbol_version_regex_str);
-
   virtual ~function_suppression();
 
   static change_kind
@@ -677,15 +654,7 @@ public:
 
   priv_sptr priv_;
 
-  variable_suppression(const string& label = "",
-		       const string& name = "",
-		       const string& name_regex_str = "",
-		       const string& symbol_name = "",
-		       const string& symbol_name_regex_str = "",
-		       const string& symbol_version = "",
-		       const string& symbol_version_regex_str = "",
-		       const string& type_name = "",
-		       const string& type_name_regex_str = "");
+  variable_suppression();
 
   virtual ~variable_suppression();
 
@@ -810,14 +779,9 @@ class file_suppression: public suppression_base
 
   priv_sptr priv_;
 
-  // Forbid this
-  file_suppression();
-
 public:
 
-  file_suppression(const string& label,
-		   const string& file_name_regex,
-		   const string& file_name_not_regex);
+  file_suppression();
 
   virtual bool
   suppresses_diff(const diff* diff) const;
diff --git a/src/abg-suppression-priv.h b/src/abg-suppression-priv.h
index deb08269c..658aeb143 100644
--- a/src/abg-suppression-priv.h
+++ b/src/abg-suppression-priv.h
@@ -497,23 +497,8 @@ struct variable_suppression::priv
   string				type_name_regex_str_;
   mutable regex::regex_t_sptr		type_name_regex_;
 
-  priv(const string& name,
-       const string& name_regex_str,
-       const string& symbol_name,
-       const string& symbol_name_regex_str,
-       const string& symbol_version,
-       const string& symbol_version_regex_str,
-       const string& type_name,
-       const string& type_name_regex_str)
-    : change_kind_(ALL_CHANGE_KIND),
-      name_(name),
-      name_regex_str_(name_regex_str),
-      symbol_name_(symbol_name),
-      symbol_name_regex_str_(symbol_name_regex_str),
-      symbol_version_(symbol_version),
-      symbol_version_regex_str_(symbol_version_regex_str),
-      type_name_(type_name),
-      type_name_regex_str_(type_name_regex_str)
+  priv()
+    : change_kind_(ALL_CHANGE_KIND)
   {}
 
   /// Getter for a pointer to a regular expression object built from
@@ -666,9 +651,14 @@ class type_suppression::priv
   mutable regex::regex_t_sptr		source_location_to_keep_regex_;
   mutable vector<string>		changed_enumerator_names_;
 
-  priv();
-
 public:
+  priv()
+    : consider_type_kind_(false),
+      type_kind_(CLASS_TYPE_KIND),
+      consider_reach_kind_(false),
+      reach_kind_(DIRECT_REACH_KIND)
+  {}
+
   priv(const string&			type_name_regexp,
        const string&			type_name,
        bool				consider_type_kind,
diff --git a/src/abg-suppression.cc b/src/abg-suppression.cc
index fb97a124b..c9b2390e0 100644
--- a/src/abg-suppression.cc
+++ b/src/abg-suppression.cc
@@ -78,33 +78,11 @@ check_sufficient_props(const char *const * names, size_t count,
 
 // <suppression_base stuff>
 
-/// Constructor for @ref suppression_base
-///
-/// @param a label for the suppression.  This represents just a
-/// comment.
-suppression_base::suppression_base(const string& label)
-  : priv_(new priv(label))
+/// Default constructor for @ref suppression_base
+suppression_base::suppression_base()
+  : priv_(new priv())
 {}
 
-/// Constructor for @ref suppression_base
-///
-/// @param a label for the suppression.  This represents just a
-/// comment.
-///
-/// @param file_name_regex_str the regular expression that denotes the
-/// file name to match.
-///
-/// @param file_name_not_regex_str the regular expression that denotes
-/// the file name to *NOT* match.
-suppression_base::suppression_base(const string& label,
-				   const string& file_name_regex_str,
-				   const string& file_name_not_regex_str)
-  : priv_(new priv(label,
-		   file_name_regex_str,
-		   file_name_not_regex_str))
-{
-}
-
 /// Tests if the current suppression specification is to avoid adding
 /// the matched ABI artifact to the internal representation or not.
 ///
@@ -457,32 +435,9 @@ read_suppressions(const string& file_path,
 
 // <type_suppression stuff>
 
-/// Constructor for @ref type_suppression.
-///
-/// @param label the label of the suppression.  This is just a free
-/// form comment explaining what the suppression is about.
-///
-/// @param type_name_regexp the regular expression describing the
-/// types about which diff reports should be suppressed.  If it's an
-/// empty string, the parameter is ignored.
-///
-/// @param type_name the name of the type about which diff reports
-/// should be suppressed.  If it's an empty string, the parameter is
-/// ignored.
-///
-/// Note that parameter @p type_name_regexp and @p type_name_regexp
-/// should not necessarily be populated.  It usually is either one or
-/// the other that the user wants.
-type_suppression::type_suppression(const string& label,
-				   const string& type_name_regexp,
-				   const string& type_name)
-  : suppression_base(label),
-    priv_(new priv(type_name_regexp,
-		   type_name,
-		   /*consider_type_kind=*/false,
-		   /*type_kind=*/CLASS_TYPE_KIND,
-		   /*consider_reach_kind=*/false,
-		   /*reach_kind=*/DIRECT_REACH_KIND))
+/// Default constructor for @ref type_suppression.
+type_suppression::type_suppression()
+  : suppression_base(), priv_(new priv)
 {}
 
 type_suppression::~type_suppression()
@@ -1910,7 +1865,10 @@ read_type_suppression(const ini::config::section& section,
 	changed_enumerator_names.push_back(p->get_value()->as_string());
     }
 
-  type_suppression result(label_str, name_regex_str, name_str);
+  type_suppression result;
+  result.set_label(label_str);
+  result.set_type_name_regex_str(name_regex_str);
+  result.set_type_name(name_str);
 
   if (consider_type_kind)
     {
@@ -2048,77 +2006,7 @@ function_suppression::parameter_spec::set_parameter_type_name_regex_str
 /// specified by using the various accessors of the @ref
 /// function_suppression type.
 function_suppression::function_suppression()
-  :  suppression_base(/*label=*/""), priv_(new priv)
-{}
-
-/// Constructor for the @ref function_suppression type.
-///
-/// @param label an informative text string that the evalution code
-/// might use to designate this function suppression specification in
-/// error messages.  This parameter might be empty, in which case it's
-/// ignored at evaluation time.
-///
-/// @param the name of the function the user wants the current
-/// specification to designate.  This parameter might be empty, in
-/// which case it's ignored at evaluation time.
-///
-/// @param nr if @p name is empty this parameter is a regular
-/// expression for a family of names of functions the user wants the
-/// current specification to designate.  If @p name is not empty, this
-/// parameter is ignored at specification evaluation time.  This
-/// parameter might be empty, in which case it's ignored at evaluation
-/// time.
-///
-/// @param ret_tn the name of the return type of the function the user
-/// wants this specification to designate.  This parameter might be
-/// empty, in which case it's ignored at evaluation time.
-///
-/// @param ret_tr if @p ret_tn is empty, then this is a regular
-/// expression for a family of return type names for functions the
-/// user wants the current specification to designate.  If @p ret_tn
-/// is not empty, then this parameter is ignored at specification
-/// evaluation time.  This parameter might be empty, in which case
-/// it's ignored at evaluation time.
-///
-/// @param ps a vector of parameter specifications to specify
-/// properties of the parameters of the functions the user wants this
-/// specification to designate.  This parameter might be empty, in
-/// which case it's ignored at evaluation time.
-///
-/// @param sym_n the name of symbol of the function the user wants
-/// this specification to designate.  This parameter might be empty,
-/// in which case it's ignored at evaluation time.
-///
-/// @param sym_nr if the parameter @p sym_n is empty, then this
-/// parameter is a regular expression for a family of names of symbols
-/// of functions the user wants this specification to designate.  If
-/// the parameter @p sym_n is not empty, then this parameter is
-/// ignored at specification evaluation time.  This parameter might be
-/// empty, in which case it's ignored at evaluation time.
-///
-/// @param sym_v the name of the version of the symbol of the function
-/// the user wants this specification to designate.  This parameter
-/// might be empty, in which case it's ignored at evaluation time.
-///
-/// @param sym_vr if the parameter @p sym_v is empty, then this
-/// parameter is a regular expression for a family of versions of
-/// symbols of functions the user wants the current specification to
-/// designate.  If the parameter @p sym_v is non empty, then this
-/// parameter is ignored.  This parameter might be empty, in which
-/// case it's ignored at evaluation time.
-function_suppression::function_suppression(const string&		label,
-					   const string&		name,
-					   const string&		nr,
-					   const string&		ret_tn,
-					   const string&		ret_tr,
-					   parameter_specs_type&	ps,
-					   const string&		sym_n,
-					   const string&		sym_nr,
-					   const string&		sym_v,
-					   const string&		sym_vr)
-  : suppression_base(label),
-    priv_(new priv(name, nr, ret_tn, ret_tr, ps,
-		   sym_n, sym_nr, sym_v, sym_vr))
+  : suppression_base(), priv_(new priv)
 {}
 
 function_suppression::~function_suppression()
@@ -3342,16 +3230,17 @@ read_function_suppression(const ini::config::section& section,
 	  parms.push_back(parm);
       }
 
-  function_suppression result(label_str,
-			      name,
-			      name_regex_str,
-			      return_type_name,
-			      return_type_regex_str,
-			      parms,
-			      sym_name,
-			      sym_name_regex_str,
-			      sym_version,
-			      sym_ver_regex_str);
+  function_suppression result;
+  result.set_label(label_str);
+  result.set_name(name);
+  result.set_name_regex_str(name_regex_str);
+  result.set_return_type_name(return_type_name);
+  result.set_return_type_regex_str(return_type_regex_str);
+  result.set_parameter_specs(parms);
+  result.set_symbol_name(sym_name);
+  result.set_symbol_name_regex_str(sym_name_regex_str);
+  result.set_symbol_version(sym_version);
+  result.set_symbol_version_regex_str(sym_ver_regex_str);
 
   if ((drop_artifact_str == "yes" || drop_artifact_str == "true")
       && (!name.empty()
@@ -3396,71 +3285,13 @@ read_function_suppression(const ini::config::section& section,
 
 // <variable_suppression stuff>
 
-/// Constructor for the @ref variable_suppression type.
-///
-/// @param label an informative text string that the evalution code
-/// might use to designate this variable suppression specification in
-/// error messages.  This parameter might be empty, in which case it's
-/// ignored at evaluation time.
-///
-/// @param name the name of the variable the user wants the current
-/// specification to designate.  This parameter might be empty, in
-/// which case it's ignored at evaluation time.
-///
-/// @param name_regex_str if @p name is empty, this parameter is a
-/// regular expression for a family of names of variables the user
-/// wants the current specification to designate.  If @p name is not
-/// empty, then this parameter is ignored at evaluation time.  This
-/// parameter might be empty, in which case it's ignored at evaluation
-/// time.
-///
-/// @param symbol_name the name of the symbol of the variable the user
-/// wants the current specification to designate.  This parameter
-/// might be empty, in which case it's ignored at evaluation time.
-///
-/// @param symbol_name_str if @p symbol_name is empty, this parameter
-/// is a regular expression for a family of names of symbols of
-/// variables the user wants the current specification to designate.
-/// If @p symbol_name is not empty, then this parameter is ignored at
-/// evaluation time.  This parameter might be empty, in which case
-/// it's ignored at evaluation time.
-///
-/// @param symbol_version the version of the symbol of the variable
-/// the user wants the current specification to designate.  This
-/// parameter might be empty, in which case it's ignored at evaluation
-/// time.
-///
-/// @param symbol_version_regex if @p symbol_version is empty, then
-/// this parameter is a regular expression for a family of versions of
-/// symbol for the variables the user wants the current specification
-/// to designate.  If @p symbol_version is not empty, then this
-/// parameter is ignored at evaluation time.  This parameter might be
-/// empty, in which case it's ignored at evaluation time.
-///
-/// @param type_name the name of the type of the variable the user
-/// wants the current specification to designate.  This parameter
-/// might be empty, in which case it's ignored at evaluation time.
+/// Default constructor for the @ref variable_suppression type.
 ///
-/// @param type_name_regex_str if @p type_name is empty, then this
-/// parameter is a regular expression for a family of type names of
-/// variables the user wants the current specification to designate.
-/// If @p type_name is not empty, then this parameter is ignored at
-/// evluation time.  This parameter might be empty, in which case it's
-/// ignored at evaluation time.
-variable_suppression::variable_suppression(const string& label,
-					   const string& name,
-					   const string& name_regex_str,
-					   const string& symbol_name,
-					   const string& symbol_name_regex_str,
-					   const string& symbol_version,
-					   const string& symbol_version_regex,
-					   const string& type_name,
-					   const string& type_name_regex_str)
-  : suppression_base(label),
-    priv_(new priv(name, name_regex_str,
-		   symbol_name, symbol_name_regex_str,
-		   symbol_version, symbol_version_regex,
-		   type_name, type_name_regex_str))
+/// It defines no suppression for now.  Suppressions have to be
+/// specified by using the various accessors of the @ref
+/// variable_suppression type.
+variable_suppression::variable_suppression()
+  : suppression_base(), priv_(new priv)
 {}
 
 /// Virtual destructor for the @erf variable_suppression type.
@@ -4199,15 +4030,16 @@ read_variable_suppression(const ini::config::section& section,
     ? type_name_regex_prop->get_value()->as_string()
      : "";
 
-  variable_suppression result(label_str,
-			      name_str,
-			      name_regex_str,
-			      symbol_name,
-			      symbol_name_regex_str,
-			      symbol_version,
-			      symbol_version_regex_str,
-			      type_name_str,
-			      type_name_regex_str);
+  variable_suppression result;
+  result.set_label(label_str);
+  result.set_name(name_str);
+  result.set_name_regex_str(name_regex_str);
+  result.set_symbol_name(symbol_name);
+  result.set_symbol_name_regex_str(symbol_name_regex_str);
+  result.set_symbol_version(symbol_version);
+  result.set_symbol_version_regex_str(symbol_version_regex_str);
+  result.set_type_name(type_name_str);
+  result.set_type_name_regex_str(type_name_regex_str);
 
   if ((drop_artifact_str == "yes" || drop_artifact_str == "true")
       && (!name_str.empty()
@@ -4248,25 +4080,8 @@ read_variable_suppression(const ini::config::section& section,
 
 // <file_suppression stuff>
 
-/// Constructor for the the @ref file_suppression type.
-///
-/// @param label the label of the suppression directive.
-///
-/// @param fname_regex_str the regular expression string that
-/// designates the file name that instances of @ref file_suppression
-/// should match.
-///
-/// @param fname_not_regex_str the regular expression string that
-/// designates the file name that instances of @ref file_suppression
-/// shoult *NOT* match.  In other words, this file_suppression should
-/// be activated if its file name does not match the regular
-/// expression @p fname_not_regex_str.
-file_suppression::file_suppression(const string& label,
-				   const string& fname_regex_str,
-				   const string& fname_not_regex_str)
-  : suppression_base(label,
-		     fname_regex_str,
-		     fname_not_regex_str)
+/// Default constructor for the the @ref file_suppression type.
+file_suppression::file_suppression()
 {}
 
 /// Test if instances of this @ref file_suppression suppresses a
@@ -4376,7 +4191,10 @@ read_file_suppression(const ini::config::section& section,
     ? soname_not_regex_prop->get_value()->as_string()
     : "";
 
-  file_suppression result(label_str, file_name_regex_str, file_name_not_regex_str);
+  file_suppression result;
+  result.set_label(label_str);
+  result.set_file_name_regex_str(file_name_regex_str);
+  result.set_file_name_not_regex_str(file_name_not_regex_str);
 
   if (!soname_regex_str.empty())
     {
diff --git a/src/abg-tools-utils.cc b/src/abg-tools-utils.cc
index 38e2f1755..2e12206d6 100644
--- a/src/abg-tools-utils.cc
+++ b/src/abg-tools-utils.cc
@@ -1794,10 +1794,8 @@ handle_file_entry(const string& file_path,
 {
   if (!suppr)
     {
-      suppr.reset(new type_suppression(get_private_types_suppr_spec_label(),
-				       /*type_name_regexp=*/"",
-				       /*type_name=*/""));
-
+      suppr.reset(new type_suppression());
+      suppr->set_label(get_private_types_suppr_spec_label());
       // Types that are defined in system headers are usually
       // OK to be considered as public types.
       suppr->set_source_location_to_keep_regex_str("^/usr/include/");
-- 
2.28.0.220.ged08abb693-goog

[PATCH 7/7] Refresh getter/setter comments.

[Giuliano Procida]

In a previous change, some useful field documentation was lost when
default constructors were removed. This patch incorporates the useful
bits into getter/setter comments, fixes some minor omissions and adds
some clarifications.

This is a document comment change only.

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

diff --git a/src/abg-suppression.cc b/src/abg-suppression.cc
index c9b2390e0..a1935385d 100644
--- a/src/abg-suppression.cc
+++ b/src/abg-suppression.cc
@@ -133,7 +133,11 @@ suppression_base::get_label() const
 
 /// Setter for the label associated to this suppression specification.
 ///
-/// @param label the new label.
+/// @param label the new label.  This is intended to be an informative
+/// text string that the evalution code might use to designate this
+/// function suppression specification in error messages.  This
+/// parameter might be empty, in which case it's ignored at evaluation
+/// time.
 void
 suppression_base::set_label(const string& label)
 {priv_->label_ = label;}
@@ -142,10 +146,11 @@ suppression_base::set_label(const string& label)
 /// of @ref suppression_base.
 ///
 /// The "file_name_regex" property is a regular expression string that
-/// designates the file name that contains the ABI artifact this
-/// suppression should apply to.
+/// identifies files containing ABI artifacts that this suppression
+/// should apply to.
 ///
-/// @param regexp the new regular expression string.
+/// @param regexp the new regular expression string that denotes the
+/// file names to match.
 void
 suppression_base::set_file_name_regex_str(const string& regexp)
 {priv_->file_name_regex_str_ = regexp;}
@@ -154,8 +159,8 @@ suppression_base::set_file_name_regex_str(const string& regexp)
 /// of @ref suppression_base.
 ///
 /// The "file_name_regex" property is a regular expression string that
-/// designates the file name that contains the ABI artifacts this
-/// suppression should apply to.
+/// identifies files containing ABI artifacts that this suppression
+/// should apply to.
 ///
 /// @return the regular expression string.
 const string&
@@ -165,12 +170,12 @@ suppression_base::get_file_name_regex_str() const
 /// Setter for the "file_name_not_regex" property of the current
 /// instance of @ref suppression_base.
 ///
-/// The current suppression specification should apply to ABI
-/// artifacts of a file which name does *NOT* match the regular
-/// expression string designated by the "file_name_not_regex"
-/// property.
+/// The "file_name_not_regex" property is a regular expression string
+/// that identifies files containing ABI artifacts that this
+/// suppression should *NOT* apply to.
 ///
-/// @param regexp the new regular expression string.
+/// @param regexp the new regular expression string that denotes the
+/// file names to *NOT* match.
 void
 suppression_base::set_file_name_not_regex_str(const string& regexp)
 {priv_->file_name_not_regex_str_ = regexp;}
@@ -178,10 +183,9 @@ suppression_base::set_file_name_not_regex_str(const string& regexp)
 /// Getter for the "file_name_not_regex" property of the current
 /// instance of @ref suppression_base.
 ///
-/// The current suppression specification should apply to ABI
-/// artifacts of a file which name does *NOT* match the regular
-/// expression string designated by the "file_name_not_regex"
-/// property.
+/// The "file_name_not_regex" property is a regular expression string
+/// that identifies files containing ABI artifacts that this
+/// suppression should *NOT* apply to.
 ///
 /// @return the regular expression string.
 const string&
@@ -471,7 +475,7 @@ type_suppression::get_type_name_regex_str() const
 /// This returns a regular expression string that specifies the family
 /// of types that should be kept after suppression.
 ///
-/// @param r the new regexp string.
+/// @param r the new regular expression.
 void
 type_suppression::set_type_name_not_regex_str(const string& r)
 {priv_->set_type_name_not_regex_str(r);}
@@ -482,7 +486,7 @@ type_suppression::set_type_name_not_regex_str(const string& r)
 /// This returns a regular expression string that specifies the family
 /// of types that should be kept after suppression.
 ///
-/// @return the new regexp string.
+/// @return the new regular expression string.
 const string&
 type_suppression::get_type_name_not_regex_str() const
 {return priv_->get_type_name_not_regex_str();}
@@ -1926,14 +1930,15 @@ read_type_suppression(const ini::config::section& section,
 /// Constructor for the @ref the function_suppression::parameter_spec
 /// type.
 ///
+/// Note that at evaluation time, the parameter @tn_regex is taken
+/// into account only if the parameter @p tn is empty.
+///
 /// @param i the index of the parameter designated by this specification.
 ///
 /// @param tn the type name of the parameter designated by this specification.
 ///
 /// @param tn_regex a regular expression that defines a set of type
-/// names for the parameter designated by this specification.  Note
-/// that at evaluation time, this regular expression is taken in
-/// account only if the parameter @p tn is empty.
+/// names for the parameter designated by this specification.
 function_suppression::parameter_spec::parameter_spec(size_t i,
 						     const string& tn,
 						     const string& tn_regex)
@@ -2069,6 +2074,10 @@ function_suppression::set_name(const string& n)
 /// Getter for a regular expression for a family of names of functions
 /// the user wants the current specification to designate.
 ///
+/// If the name as returned by function_suppression::get_name() is not
+/// empty, then this property is ignored at specification evaluation
+/// time.
+///
 /// @return the regular expression for the possible names of the
 /// function(s).
 const string&
@@ -2078,6 +2087,10 @@ function_suppression::get_name_regex_str() const
 /// Setter for a regular expression for a family of names of functions
 /// the user wants the current specification to designate.
 ///
+/// If the name as returned by function_suppression::get_name() is not
+/// empty, then this property is ignored at specification evaluation
+/// time.
+///
 /// @param r the new the regular expression for the possible names of
 /// the function(s).
 void
@@ -3355,6 +3368,7 @@ variable_suppression::set_name(const string& n)
 
 /// Getter for the regular expression for a family of names of
 /// variables the user wants the current specification to designate.
+///
 /// If the variable name as returned by
 /// variable_suppression::get_name() is not empty, then this property
 /// is ignored at evaluation time.  This property might be empty, in
@@ -3367,6 +3381,7 @@ variable_suppression::get_name_regex_str() const
 
 /// Setter for the regular expression for a family of names of
 /// variables the user wants the current specification to designate.
+///
 /// If the variable name as returned by
 /// variable_suppression::get_name() is not empty, then this property
 /// is ignored at evaluation time.  This property might be empty, in
@@ -3386,7 +3401,7 @@ variable_suppression::get_name_not_regex_str() const
 
 /// Setter for the "name_not_regexp" property of the specification.
 ///
-/// @param r the new value of the "name_not_regexp" property.
+/// @param r the new regular expression for variable name exclusion.
 void
 variable_suppression::set_name_not_regex_str(const string& r)
 {priv_->name_not_regex_str_ = r;}
@@ -3498,7 +3513,10 @@ variable_suppression::set_symbol_version(const string& v)
 
 /// Getter of the regular expression for a family of versions of
 /// symbol for the variables the user wants the current specification
-/// to designate.  If @p symbol_version is not empty, then this
+/// to designate.
+///
+/// If the symbol version as returned by
+/// variable_suppression::get_symbol_version() is not empty, then this
 /// property is ignored at evaluation time.  This property might be
 /// empty, in which case it's ignored at evaluation time.
 ///
@@ -3510,7 +3528,10 @@ variable_suppression::get_symbol_version_regex_str() const
 
 /// Setter of the regular expression for a family of versions of
 /// symbol for the variables the user wants the current specification
-/// to designate.  If @p symbol_version is not empty, then this
+/// to designate.
+///
+/// If the symbol version as returned by
+/// variable_suppression::get_symbol_version() is not empty, then this
 /// property is ignored at evaluation time.  This property might be
 /// empty, in which case it's ignored at evaluation time.
 ///
-- 
2.28.0.220.ged08abb693-goog

Re: [PATCH 1/7] Add missing newlines to end of test files.

[Dodji Seketeli]

Hello Giuliano,

Giuliano Procida <gprocida@google.com> a écrit:

> Various test files were missing terminal newlines.
>
> 	* tests/data/test-diff-suppr/test0-type-suppr-2.suppr: Add
> 	final new line.
> 	* tests/data/test-diff-suppr/test22-suppr-removed-var-sym-4.suppr:
> 	Likewise.
> 	* tests/data/test-diff-suppr/test23-alias-filter-0.suppr:
> 	Likewise.
> 	* tests/data/test-diff-suppr/test23-alias-filter-4.suppr:
> 	Likewise.
> 	* tests/data/test-diff-suppr/test28-add-aliased-function-1.suppr:
> 	Likewise.
> 	* tests/data/test-diff-suppr/test28-add-aliased-function-2.suppr:
> 	Likewise.
> 	* tests/data/test-diff-suppr/test28-add-aliased-function-3.suppr:
> 	Likewise.
> 	* tests/data/test-diff-suppr/test28-add-aliased-function-4.suppr:
> 	Likewise.
> 	* tests/data/test-diff-suppr/test41-enumerator-changes-0.suppr:
> 	Likewise.
> 	* tests/data/test-diff-suppr/test7-var-suppr-7.suppr:
> 	Likewise.
> 	* tests/data/test-ini/test01-equal-in-property-string.abignore:
> 	Likewise.
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>

Applied to master, thanks!

Cheers,

[...]

-- 
		Dodji

Re: [PATCH 2/7] Fix two wrongs in test suppression regex

[Dodji Seketeli]

Giuliano Procida <gprocida@google.com> a écrit:

> The suppression specification in test38-char-class-in-ini.abignore was
> introduced in commit 1478d9cc1c74ca3a2be89cd09aac5afcbdf818c7.
>
> Unfortunately it contains two errors. One causes the file name not to
> match as the string is the full path, not the base name. The other is
> a typo that causes the file name match not to even be attempted. The
> two mistakes cancel in the test, but result in a suppression
> specification that is broader than intended.
>
> 	* tests/data/test-diff-suppr/test38-char-class-in-ini.abignore:
> 	Don't anchor regex match to beginning of file name.
> 	Change "filename_regexp" to "file_name_regexp".
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>

Applied to master, thanks!

[...]

Cheers,

-- 
		Dodji

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

[Dodji Seketeli]

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

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

[Giuliano Procida]

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
>

Re: [PATCH 0/7] Suppression parsing - preparatory work

[Dodji Seketeli]

Hello Giuliano,

Giuliano Procida <gprocida@google.com> a écrit:

> Quite a while ago I had a series of patches with the aim of improving
> libabigail's suppression parsing with the main aims:
>
> * adding error handling and reporting
> * refactoring for easier maintenance (both fixes and features)
>
> Early on in the series, I changed the way regexes were parsed and
> passed in and out of the suppression specifications. This wasn't
> something you were happy with, so I shelved the series.
>
> I've taken a lttle time to remove those changes and rebase the series.
> My plan is to feed changes to you in digistible batches.
>
> This batch contains:
>
> * 2 commits that fix issues in an uncontroversial way

Thanks!  I happily applied these.

> * 3 commits to add the outer shell of error handling
> * 2 commits to simplify how suppressions are constructed
>
> The error handling commits do not add any error reporting but do add
> placeholder TODOs for where this could be added.
>
> The constructor change commits remove the non-default constructors for
> the 4 suppression types as they are antithetical to a table-driver
> parser where there are a large number of optional fields.

I do really prefer that we keep recursive descent parsers in all the
parsers of the project.  Why? Because they are the simplest parsers to
understand for someone who just wants to /debug/ it to fix things in the
way the parser.  Yes, they are more verbose and the grammar handling is
hard coded.  But that's a tradeoff I accept to keep the whole project as
"debuggable" as it can be, given the inherent complexity of the core
subject we are trying to tackle here.

So, yeah, I am really not a fan for introducing a table-driven parser in
this context at this point.

[...]

Cheers,

-- 
		Dodji

Re: [PATCH 0/7] Suppression parsing - preparatory work

[Giuliano Procida]

Hi there.

On Fri, 2 Oct 2020 at 22:53, Dodji Seketeli <dodji@seketeli.org> wrote:

> Hello Giuliano,
>
> Giuliano Procida <gprocida@google.com> a écrit:
>
> > Quite a while ago I had a series of patches with the aim of improving
> > libabigail's suppression parsing with the main aims:
> >
> > * adding error handling and reporting
> > * refactoring for easier maintenance (both fixes and features)
> >
> > Early on in the series, I changed the way regexes were parsed and
> > passed in and out of the suppression specifications. This wasn't
> > something you were happy with, so I shelved the series.
> >
> > I've taken a lttle time to remove those changes and rebase the series.
> > My plan is to feed changes to you in digistible batches.
> >
> > This batch contains:
> >
> > * 2 commits that fix issues in an uncontroversial way
>
> Thanks!  I happily applied these.
>
> > * 3 commits to add the outer shell of error handling
> > * 2 commits to simplify how suppressions are constructed
> >
> > The error handling commits do not add any error reporting but do add
> > placeholder TODOs for where this could be added.
> >
> > The constructor change commits remove the non-default constructors for
> > the 4 suppression types as they are antithetical to a table-driver
> > parser where there are a large number of optional fields.
>
> I do really prefer that we keep recursive descent parsers in all the
> parsers of the project.  Why? Because they are the simplest parsers to
> understand for someone who just wants to /debug/ it to fix things in the
> way the parser.  Yes, they are more verbose and the grammar handling is
> hard coded.  But that's a tradeoff I accept to keep the whole project as
> "debuggable" as it can be, given the inherent complexity of the core
> subject we are trying to tackle here.
>
> So, yeah, I am really not a fan for introducing a table-driven parser in
> this context at this point.
>

Understood.

I've mothballed this line of development (see
https://sourceware.org/bugzilla/show_bug.cgi?id=19608#c3).

If someone wants to try an alternative approach, they will at least get an
idea of the scale of the changes needed.


>
> [...]
>
> Cheers,
>

Regards,
Giuliano.


>
> --
>                 Dodji
>