diff mbox series

suppression: Add "changed_enumerators_regexp" property

Message ID	20230928174344.8268-1-quic_johmoo@quicinc.com
State	New
Headers	DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org AFD313858C52 To: <libabigail@sourceware.org>, Dodji Seketeli <dodji@seketeli.org> Subject: [PATCH] suppression: Add "changed_enumerators_regexp" property Date: Thu, 28 Sep 2023 10:43:44 -0700 Message-ID: <20230928174344.8268-1-quic_johmoo@quicinc.com> MIME-Version: 1.0 Content-Type: text/plain Precedence: list From: John Moon via Libabigail <libabigail@sourceware.org> Reply-To: John Moon <quic_johmoo@quicinc.com> Cc: Trilok Soni <quic_tsoni@quicinc.com>, Satya Durga Srinivasu Prabhala <quic_satyap@quicinc.com> Errors-To: libabigail-bounces+patchwork=sourceware.org@sourceware.org Sender: "Libabigail" <libabigail-bounces+patchwork=sourceware.org@sourceware.org>
Series	suppression: Add "changed_enumerators_regexp" property \| suppression: Add "changed_enumerators_regexp" property

Commit Message

John Moon Sept. 28, 2023, 5:43 p.m. UTC

  Currently, users are able to suppress changes to enumerator variants
matching names specified in "changed_enumerators", but can't specify
regular expressions.

A common pattern when using enums is to have the final enum variant
labeled as "*_MAX" or "*_LAST" so that users of the enum can have a
way to determine how many variants there are. In these cases, when
expanding an enum, the last variant will change, but that's an
expected result that users may not want to flag as an ABI-breaking
change.

For example, if you have a change like this:

enum foo {
        FOO,
        BAR,
        FOOBAR_MAX,
};

...

enum foo {
        FOO,
        BAR,
        BAZ,
        FOOBAR_MAX,
};

abidiff reports:

1 changed type unreachable from any public interface:

  [C] 'enum foo' changed:
    type size hasn't changed
    1 enumerator insertion:
      'foo::BAZ' value '2'
    1 enumerator change:
      'foo::FOOBAR_MAX' from value '2' to '3' at test_2.c:1:1

With a new changed_enumerators_regexp property, users can specify a
suppression which stops abidiff from emitting this diff for any
members which match the regular expressions in the list:

[suppress_type]
  type_kind = enum
  changed_enumerators_regexp = .*_MAX$, .*_LAST$, .*_NUM$, .*_NBITS$

Signed-off-by: John Moon <quic_johmoo@quicinc.com>
---
 include/abg-suppression.h  |  6 +++
 src/abg-suppression-priv.h |  1 +
 src/abg-suppression.cc     | 84 +++++++++++++++++++++++++++++++++-----
 3 files changed, 81 insertions(+), 10 deletions(-)

--
2.17.1

Comments

Dodji Seketeli Sept. 30, 2023, 6:56 a.m. UTC | #1

Hello John,

> Currently, users are able to suppress changes to enumerator variants
> matching names specified in "changed_enumerators", but can't specify
> regular expressions.
> 
> A common pattern when using enums is to have the final enum variant
> labeled as "*_MAX" or "*_LAST" so that users of the enum can have a
> way to determine how many variants there are. In these cases, when
> expanding an enum, the last variant will change, but that's an
> expected result that users may not want to flag as an ABI-breaking
> change.
> 
> For example, if you have a change like this:
> 
> enum foo {
>         FOO,
>         BAR,
>         FOOBAR_MAX,
> };
> 
> ...
> 
> enum foo {
>         FOO,
>         BAR,
>         BAZ,
>         FOOBAR_MAX,
> };
> 
> abidiff reports:
> 
> 1 changed type unreachable from any public interface:
> 
>   [C] 'enum foo' changed:
>     type size hasn't changed
>     1 enumerator insertion:
>       'foo::BAZ' value '2'
>     1 enumerator change:
>       'foo::FOOBAR_MAX' from value '2' to '3' at test_2.c:1:1
> 
> With a new changed_enumerators_regexp property, users can specify a
> suppression which stops abidiff from emitting this diff for any
> members which match the regular expressions in the list:
> 
> [suppress_type]
>   type_kind = enum
>   changed_enumerators_regexp = .*_MAX$, .*_LAST$, .*_NUM$, .*_NBITS$

Nice.  It goes without saying that this is a step closer in the
support for check-uapi.sh.  Thank you for working on this!

The patch looks good to me in general.  I do have some casual
observations that I have made below.

[...]

diff --git a/include/abg-suppression.h b/include/abg-suppression.h

[...]

> +  const vector<regex::regex_t_sptr>&
> +  get_changed_enumerator_regexp() const;
> +
> +  void
> +  set_changed_enumerator_regexp(const vector<regex::regex_t_sptr>&);
> +

As the property you've added to the suppression specification
"language" is "changed_enumerators_regexp", (note the 's' at the end
of enumerator), I'd say that the name of these accessor member
functions you are declaring should rather be
'get_changed_enumerators_regexp' and 'set_changed_enumerators_regexp'
(note the 's' at the end enumerator here as well), for the sake of
consistency.

[...]


> diff --git a/src/abg-suppression-priv.h b/src/abg-suppression-priv.h

[...]

> @@ -585,6 +585,7 @@ class type_suppression::priv
>    string				source_location_to_keep_regex_str_;
>    mutable regex::regex_t_sptr		source_location_to_keep_regex_;
>    mutable vector<string>		changed_enumerator_names_;
> +  mutable vector<regex::regex_t_sptr>	changed_enumerator_regexp_;

Likewise, I'd named this 'changed_enumerators_regexp', with an 's' at
the end of 'enumerator'.

[...]


> diff --git a/src/abg-suppression.cc b/src/abg-suppression.cc

[...]


> +/// Getter of the vector of the regular expression strings for changed
> +/// enumerators that are supposed to be suppressed. Note that this
> +/// will be "valid" only if the type suppression has the
> +/// 'type_kind = enum' property.
> +///
> +/// @return the vector of the regular expression strings that are
> +/// supposed to match enumertor names to be suppressed.

Thank you for adding comments to these functions.  This is appreciated.

[...]

> +const vector<regex::regex_t_sptr>&
> +type_suppression::get_changed_enumerator_regexp() const

As a consequence of a comment I made earlier, I'd add an 's' to
enumerator, for the sake of consistency.

[...]

> +void
> +type_suppression::set_changed_enumerator_regexp(const vector<regex::regex_t_sptr>& n)

Likewise.

[...]

> @@ -996,9 +1018,12 @@ type_suppression::suppresses_diff(const diff* diff) const
>        // ... and yet carries some changed enumerators!
>        && !enum_dif->changed_enumerators().empty())
>      {
> -      // Make sure that all changed enumerators are listed in the
> -      // vector of enumerator names returned by the
> -      // get_changed_enumerator_names() member function.
> +
> +      // Make sure that all changed enumerators are either:
> +      //  1. listed in the vector of enumerator names returned
> +      //     by the get_changed_enumerator_names() member function
> +      //  2. match a regular expression returned by the
> +      //     get_changed_enumerator_regexp() member function
>        bool matched = true;
>        for (string_changed_enumerator_map::const_iterator i =
>  	     enum_dif->changed_enumerators().begin();
> @@ -1006,13 +1031,20 @@ type_suppression::suppresses_diff(const diff* diff) const
>  	   ++i)
>  	{
>  	  matched &= true;
> -	  if (std::find(get_changed_enumerator_names().begin(),
> -			get_changed_enumerator_names().end(),
> -			i->first) == get_changed_enumerator_names().end())
> -	    {
> -	      matched &= false;
> -	      break;
> -	    }
> +	  if ((std::find(get_changed_enumerator_names().begin(),
> +			 get_changed_enumerator_names().end(),
> +			 i->first) == get_changed_enumerator_names().end())
> +	     &&
> +	      (std::find_if(get_changed_enumerator_regexp().begin(),
> +			    get_changed_enumerator_regexp().end(),
> +			    [&] (const regex_t_sptr& enum_regexp)
> +		{
> +		  return regex::match(enum_regexp, i->first);
> +		}) == get_changed_enumerator_regexp().end()))
> +	  {
> +	    matched &= false;
> +	    break;
> +	  }

Nicely nailed.

[...]


> @@ -2154,6 +2186,34 @@ read_type_suppression(const ini::config::section& section)
>  	changed_enumerator_names.push_back(p->get_value()->as_string());
>      }
> 
> +  /// Support 'changed_enumerators_regexp = .*_foo, bar_[0-9]+, baz'
> +  ///
> +  /// If the current type is an enum and if it carries changed
> +  /// enumerators that match regular expressions listed in the
> +  /// changed_enumerator_regexp property value then it should be
> +  /// suppressed.
> +
> +  ini::property_sptr changed_enumerators_regexp_prop =
> +    section.find_property("changed_enumerators_regexp");
> +
> +  vector<regex_t_sptr> changed_enumerator_regexp;
> +  if (changed_enumerators_regexp_prop)
> +    {
> +      if (ini::list_property_sptr p =
> +	  is_list_property(changed_enumerators_regexp_prop))
> +      {
> +	for (string e : p->get_value()->get_content())
> +	  changed_enumerator_regexp.push_back(regex::compile(e));
> +      }

Nice.

Please note that in earlier cases where we expect regular expressions,
we store the regex *string* in a data member of
type_suppression::priv, before compiling it.  Here, you compile the
string into a regex directly.  It's fine too.  If at some point in the
future we need to regex string, we'll store it.

> +      else if (ini::simple_property_sptr p =
> +	       is_simple_property(changed_enumerators_prop))

I think there is a (copy-&-paste) typo here.  It should probably not
be 'changed_enumerators_prop', but rather
'changed_enumerators_regexp_prop'.  This is to support the case where
there is just one regular expression (as opposed to a list) provided
as the value of the property.

[...]

OK, this is neatly handled with great care, thanks for that.

There are however some missing (boring, sorry) pieces: documentation
and test cases.  I understand that this patch is work in progress, so
this is really not a problem.

The documentation is for the new 'changed_enumerators_regexp' property
of the suppression specification language.  It should be added to the
file doc/manuals/libabigail-concepts.rst, browsable at
https://sourceware.org/git/?p=libabigail.git;a=blob;f=doc/manuals/libabigail-concepts.rst.

Test cases are a little bit more involved to get into first, but with
practice, it's should not be hard.

For suppression specifications, the tests are run by the program at
tests/test-diff-suppr.cc, for instance. What the program does is
described briefly by a comment at the beginning of the file.

In a few words, there is an array of entries represented by the
variable "in_out_specs", that defines a set of binaries that are
compared.  The result of the comparison should exactly match an output file
that is also defined in the entry.  The entry defines a suppression
specification file that should be applied to the comparison.

Finally, there is a last bit of (redundant) bit of documentation that
is expected to be added to the patch, at the end of the commit log,
which we call the "ChangeLog" part of the commit log.  Its format
follows the GNU ChangeLog format (what you get by doing M-x
change-log-mode in emacs).  Please note that the format of the entire
commit log that we expect is documented in the file
'COMMIT-LOG-GUIDELINES', browseable at
https://sourceware.org/git/?p=libabigail.git;a=blob;f=COMMIT-LOG-GUIDELINES.

The main reason why we require this is that its inherent redundancy
helps catch a number of errors during the reviews we make ourselves
before submitting the patches.

Finally, to test the whole thing, you can type:

    $ make distcheck -C <build-directory> -j<number-of-cores>

(or make distcheck-fast to go a little bit faster).

So, long story short, I have amended your patch to add the
documentation to doc/manuals/libabigail-concepts.rst, tests to the
testsuite and I have also added a ChangeLog part to the commit log and
apply the changes I suggested during this review.

Please find below the resulting patch.  Maybe you could test it in your
environment and review it again, to catch potential issues I could
have introduced.

In any case, thank you very much for caring so much.

----------------------------------->8<--------------------------------
From 227fbc30e227c03876176e90eec8ac3418617cb4 Mon Sep 17 00:00:00 2001
From: John Moon <quic_johmoo@quicinc.com>
Date: Thu, 28 Sep 2023 10:43:44 -0700
Subject: [PATCH] suppression: Add "changed_enumerators_regexp" property

Currently, users are able to suppress changes to enumerator variants
matching names specified in "changed_enumerators", but can't specify
regular expressions.

A common pattern when using enums is to have the final enum variant
labeled as "*_MAX" or "*_LAST" so that users of the enum can have a
way to determine how many variants there are. In these cases, when
expanding an enum, the last variant will change, but that's an
expected result that users may not want to flag as an ABI-breaking
change.

For example, if you have a change like this:

enum foo {
        FOO,
        BAR,
        FOOBAR_MAX,
};

...

enum foo {
        FOO,
        BAR,
        BAZ,
        FOOBAR_MAX,
};

abidiff reports:

1 changed type unreachable from any public interface:

  [C] 'enum foo' changed:
    type size hasn't changed
    1 enumerator insertion:
      'foo::BAZ' value '2'
    1 enumerator change:
      'foo::FOOBAR_MAX' from value '2' to '3' at test_2.c:1:1

With a new changed_enumerators_regexp property, users can specify a
suppression which stops abidiff from emitting this diff for any
members which match the regular expressions in the list:

[suppress_type]
  type_kind = enum
  changed_enumerators_regexp = .*_MAX$, .*_LAST$, .*_NUM$, .*_NBITS$

	* include/abg-suppression.h
	(type_suppression::{g,s}et_changed_enumerators_regexp): Declare new
	accessor functions.
	* src/abg-suppression-priv.h
	(type_suppression::priv::changed_enumerators_regexp_): Define new
	data member.
	* src/abg-suppression.cc
	(type_suppression::{g,s}et_changed_enumerators_regexp): Define new
	accessor function.
	(type_suppression::suppresses_diff): For a type suppression to
	match an enum_diff, the names of all changed enumerators must
	match either the names returned by
	type_suppression::get_changed_enumerator_names or one of the
	regexps returned by the new member function
	type_suppression::get_changed_enumerators_regexp.
	(read_type_suppression): Parse the new
	'changed_enumerators_regexp' property to set the
	type_suppression::get_changed_enumerators_regexp property.
	* doc/manuals/libabigail-concepts.rst: Add an entry for the new
	'changed_enumerators_regexp' property.
	* tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-[1-5].suppr:
	Add new test suppression files.
	* tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-[0-5].txt:
	Add new test reference output files.
	* tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v{0,1}.c:
	Add source code for new binary test input files.
	* tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v{0,1}.o:
	Add new binary test input files.
	* tests/data/Makefile.am: Add the new test files to the source
	distribution.
	* tests/test-diff-suppr.cc (in_out_specs): Add the new test input
	files to this test harness.

Signed-off-by: John Moon <quic_johmoo@quicinc.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 doc/manuals/libabigail-concepts.rst           |  24 +++++
 include/abg-suppression.h                     |   6 ++
 src/abg-suppression-priv.h                    |   1 +
 src/abg-suppression.cc                        |  84 +++++++++++++++---
 tests/data/Makefile.am                        |  15 ++++
 ...merator-changes-enumerator-changes-1.suppr |   3 +
 ...merator-changes-enumerator-changes-2.suppr |   3 +
 ...merator-changes-enumerator-changes-3.suppr |   3 +
 ...merator-changes-enumerator-changes-4.suppr |   3 +
 ...merator-changes-enumerator-changes-5.suppr |   3 +
 ...or-changes-enumerator-changes-report-0.txt |  29 ++++++
 ...or-changes-enumerator-changes-report-1.txt |  21 +++++
 ...or-changes-enumerator-changes-report-2.txt |  21 +++++
 ...or-changes-enumerator-changes-report-3.txt |  13 +++
 ...or-changes-enumerator-changes-report-4.txt |  21 +++++
 ...or-changes-enumerator-changes-report-5.txt |   3 +
 ...enumerator-changes-enumerator-changes-v0.c |  37 ++++++++
 ...enumerator-changes-enumerator-changes-v0.o | Bin 0 -> 3944 bytes
 ...enumerator-changes-enumerator-changes-v1.c |  40 +++++++++
 ...enumerator-changes-enumerator-changes-v1.o | Bin 0 -> 4064 bytes
 tests/test-diff-suppr.cc                      |  60 +++++++++++++
 21 files changed, 380 insertions(+), 10 deletions(-)
 create mode 100644 tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-1.suppr
 create mode 100644 tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-2.suppr
 create mode 100644 tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-3.suppr
 create mode 100644 tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-4.suppr
 create mode 100644 tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-5.suppr
 create mode 100644 tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-0.txt
 create mode 100644 tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-1.txt
 create mode 100644 tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-2.txt
 create mode 100644 tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-3.txt
 create mode 100644 tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-4.txt
 create mode 100644 tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-5.txt
 create mode 100644 tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v0.c
 create mode 100644 tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v0.o
 create mode 100644 tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v1.c
 create mode 100644 tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v1.o

diff --git a/doc/manuals/libabigail-concepts.rst b/doc/manuals/libabigail-concepts.rst
index 4f26932c..225d61eb 100644
--- a/doc/manuals/libabigail-concepts.rst
+++ b/doc/manuals/libabigail-concepts.rst
@@ -730,6 +730,30 @@ changes, unless the ``has_size_changes`` property is set to ``yes``.
 
       changed_enumerators = LAST_ENUMERATORS0, LAST_ENUMERATOR1
 
+.. _suppr_changed_enumerators_regexp_label:
+
+* ``changed_enumerators_regexp``
+
+  Usage:
+
+    ``changed_enumerators_regexp`` ``=`` <list-of-enumerator-regular-expressions>
+    
+  Suppresses change reports involving changes in the value of
+  enumerators of a given enum type.  This property is applied if the
+  ``type_kind`` property is set to the value ``enum``, at least.  The
+  value of the ``changed_enumerators_regexp`` property is a
+  comma-separated list of regular expressions that should match the
+  names of the enumerators that the user expects to change.  For
+  instance: ::
+
+      changed_enumerators_regexp = .*_MAX$, .*_LAST$, .*_NUM$, .*_NBITS$
+
+  In the example above, change reports to any enumerator which name
+  ends with _MAX, _LAST, _NUM or _NBITS will be suppressed.
+
+  Note that for this property to be applied to changes to an enum
+  type, the size of the enum type must *NOT* have changed.
+
 ``[suppress_function]``
 $$$$$$$$$$$$$$$$$$$$$$$$
 
diff --git a/include/abg-suppression.h b/include/abg-suppression.h
index 9449d06d..d6283eb8 100644
--- a/include/abg-suppression.h
+++ b/include/abg-suppression.h
@@ -330,6 +330,12 @@ public:
   void
   set_changed_enumerator_names(const vector<string>&);
 
+  const vector<regex::regex_t_sptr>&
+  get_changed_enumerators_regexp() const;
+
+  void
+  set_changed_enumerators_regexp(const vector<regex::regex_t_sptr>&);
+
   virtual bool
   suppresses_diff(const diff* diff) const;
 
diff --git a/src/abg-suppression-priv.h b/src/abg-suppression-priv.h
index cf66a9f1..351c5965 100644
--- a/src/abg-suppression-priv.h
+++ b/src/abg-suppression-priv.h
@@ -585,6 +585,7 @@ class type_suppression::priv
   string				source_location_to_keep_regex_str_;
   mutable regex::regex_t_sptr		source_location_to_keep_regex_;
   mutable vector<string>		changed_enumerator_names_;
+  mutable vector<regex::regex_t_sptr>	changed_enumerators_regexp_;
 
   priv();
 
diff --git a/src/abg-suppression.cc b/src/abg-suppression.cc
index 4ed42e82..22adfa8c 100644
--- a/src/abg-suppression.cc
+++ b/src/abg-suppression.cc
@@ -770,6 +770,28 @@ void
 type_suppression::set_changed_enumerator_names(const vector<string>& n)
 {priv_->changed_enumerator_names_ = n;}
 
+/// Getter of the vector of the regular expression strings for changed
+/// enumerators that are supposed to be suppressed. Note that this
+/// will be "valid" only if the type suppression has the
+/// 'type_kind = enum' property.
+///
+/// @return the vector of the regular expression strings that are
+/// supposed to match enumertor names to be suppressed.
+const vector<regex::regex_t_sptr>&
+type_suppression::get_changed_enumerators_regexp() const
+{return priv_->changed_enumerators_regexp_;}
+
+/// Setter of the vector of the regular expression strings for changed
+/// enumerators that are supposed to be suppressed. Note that this
+/// will be "valid" only if the type suppression has the
+/// 'type_kind = enum' property.
+///
+/// @param n the vector of the regular expression strings that are
+/// supposed to match enumertor names to be suppressed.
+void
+type_suppression::set_changed_enumerators_regexp(const vector<regex::regex_t_sptr>& n)
+{priv_->changed_enumerators_regexp_ = n;}
+
 /// Evaluate this suppression specification on a given diff node and
 /// say if the diff node should be suppressed or not.
 ///
@@ -996,9 +1018,12 @@ type_suppression::suppresses_diff(const diff* diff) const
       // ... and yet carries some changed enumerators!
       && !enum_dif->changed_enumerators().empty())
     {
-      // Make sure that all changed enumerators are listed in the
-      // vector of enumerator names returned by the
-      // get_changed_enumerator_names() member function.
+
+      // Make sure that all changed enumerators are either:
+      //  1. listed in the vector of enumerator names returned
+      //     by the get_changed_enumerator_names() member function
+      //  2. match a regular expression returned by the
+      //     get_changed_enumerators_regexp() member function
       bool matched = true;
       for (string_changed_enumerator_map::const_iterator i =
 	     enum_dif->changed_enumerators().begin();
@@ -1006,13 +1031,20 @@ type_suppression::suppresses_diff(const diff* diff) const
 	   ++i)
 	{
 	  matched &= true;
-	  if (std::find(get_changed_enumerator_names().begin(),
-			get_changed_enumerator_names().end(),
-			i->first) == get_changed_enumerator_names().end())
-	    {
-	      matched &= false;
-	      break;
-	    }
+	  if ((std::find(get_changed_enumerator_names().begin(),
+			 get_changed_enumerator_names().end(),
+			 i->first) == get_changed_enumerator_names().end())
+	     &&
+	      (std::find_if(get_changed_enumerators_regexp().begin(),
+			    get_changed_enumerators_regexp().end(),
+			    [&] (const regex_t_sptr& enum_regexp)
+		{
+		  return regex::match(enum_regexp, i->first);
+		}) == get_changed_enumerators_regexp().end()))
+	  {
+	    matched &= false;
+	    break;
+	  }
 	}
       if (!matched)
 	return false;
@@ -2154,6 +2186,34 @@ read_type_suppression(const ini::config::section& section)
 	changed_enumerator_names.push_back(p->get_value()->as_string());
     }
 
+  /// Support 'changed_enumerators_regexp = .*_foo, bar_[0-9]+, baz'
+  ///
+  /// If the current type is an enum and if it carries changed
+  /// enumerators that match regular expressions listed in the
+  /// changed_enumerators_regexp property value then it should be
+  /// suppressed.
+
+  ini::property_sptr changed_enumerators_regexp_prop =
+    section.find_property("changed_enumerators_regexp");
+
+  vector<regex_t_sptr> changed_enumerators_regexp;
+  if (changed_enumerators_regexp_prop)
+    {
+      if (ini::list_property_sptr p =
+	  is_list_property(changed_enumerators_regexp_prop))
+      {
+	for (string e : p->get_value()->get_content())
+	  changed_enumerators_regexp.push_back(regex::compile(e));
+      }
+      else if (ini::simple_property_sptr p =
+	       is_simple_property(changed_enumerators_regexp_prop))
+      {
+	changed_enumerators_regexp.push_back(
+	  regex::compile(p->get_value()->as_string())
+	);
+      }
+    }
+
   if (section.get_name() == "suppress_type")
     result.reset(new type_suppression(label_str, name_regex_str, name_str));
   else if (section.get_name() == "allow_type")
@@ -2217,6 +2277,10 @@ read_type_suppression(const ini::config::section& section)
       && !changed_enumerator_names.empty())
     result->set_changed_enumerator_names(changed_enumerator_names);
 
+  if (result->get_type_kind() == type_suppression::ENUM_TYPE_KIND
+      && !changed_enumerators_regexp.empty())
+    result->set_changed_enumerators_regexp(changed_enumerators_regexp);
+
   return result;
 }
 
diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index b3434da5..1632e2ab 100644
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -1755,6 +1755,21 @@ test-diff-suppr/test39-public-headers-dir/test39-header-v0.h \
 test-diff-suppr/test39-public-headers-dir/test39-header-v1.h \
 test-diff-suppr/libtest40-enumerator-changes-v0.so \
 test-diff-suppr/libtest40-enumerator-changes-v1.so \
+test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-1.suppr \
+test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-2.suppr \
+test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-3.suppr \
+test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-4.suppr \
+test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-5.suppr \
+test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-0.txt \
+test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-1.txt \
+test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-2.txt \
+test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-3.txt \
+test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-4.txt \
+test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-5.txt \
+test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v0.c \
+test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v1.c \
+test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v0.o \
+test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v1.o \
 test-diff-suppr/libtest41-enumerator-changes-v0.so \
 test-diff-suppr/libtest41-enumerator-changes-v1.so \
 test-diff-suppr/test40-enumerator-changes-0.suppr \
diff --git a/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-1.suppr b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-1.suppr
new file mode 100644
index 00000000..cbe76ab8
--- /dev/null
+++ b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-1.suppr
@@ -0,0 +1,3 @@
+[suppress_type]
+  type_kind = enum
+  changed_enumerators_regexp = ^.*_LAST$
diff --git a/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-2.suppr b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-2.suppr
new file mode 100644
index 00000000..0ab63c10
--- /dev/null
+++ b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-2.suppr
@@ -0,0 +1,3 @@
+[suppress_type]
+  type_kind = enum
+  changed_enumerators_regexp = ^.*_MAX$
diff --git a/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-3.suppr b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-3.suppr
new file mode 100644
index 00000000..e8bb9117
--- /dev/null
+++ b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-3.suppr
@@ -0,0 +1,3 @@
+[suppress_type]
+  type_kind = enum
+  changed_enumerators_regexp = ^.*_LAST$, ^.*_MAX$
\ No newline at end of file
diff --git a/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-4.suppr b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-4.suppr
new file mode 100644
index 00000000..492dd336
--- /dev/null
+++ b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-4.suppr
@@ -0,0 +1,3 @@
+[suppress_type]
+  type_kind = enum
+  changed_enumerators_regexp = ^LAST_.*$
\ No newline at end of file
diff --git a/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-5.suppr b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-5.suppr
new file mode 100644
index 00000000..3ee5d407
--- /dev/null
+++ b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-5.suppr
@@ -0,0 +1,3 @@
+[suppress_type]
+  type_kind = enum
+  changed_enumerators_regexp = ^.*_LAST$, ^.*_MAX$, ^LAST_.*$
\ No newline at end of file
diff --git a/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-0.txt b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-0.txt
new file mode 100644
index 00000000..26435de3
--- /dev/null
+++ b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-0.txt
@@ -0,0 +1,29 @@
+Functions changes summary: 0 Removed, 3 Changed, 0 Added functions
+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
+
+3 functions with some indirect sub-type change:
+
+  [C] 'function void fun0(Enum0)' at test40.1-enumerator-changes-enumerator-changes-v0.c:25:1 has some indirect sub-type changes:
+    parameter 1 of type 'enum Enum0' has sub-type changes:
+      type size hasn't changed
+      1 enumerator insertion:
+        'Enum0::ENUM0_E1' value '1'
+      1 enumerator change:
+        'Enum0::ENOM0_LAST' from value '1' to '2' at test40.1-enumerator-changes-enumerator-changes-v1.c:6:1
+
+  [C] 'function void fun1(Enum1)' at test40.1-enumerator-changes-enumerator-changes-v0.c:30:1 has some indirect sub-type changes:
+    parameter 1 of type 'enum Enum1' has sub-type changes:
+      type size hasn't changed
+      1 enumerator insertion:
+        'Enum1::ENUM1_E1' value '1'
+      1 enumerator change:
+        'Enum1::ENUM1_MAX' from value '1' to '2' at test40.1-enumerator-changes-enumerator-changes-v1.c:13:1
+
+  [C] 'function void fun2(Enum2)' at test40.1-enumerator-changes-enumerator-changes-v0.c:35:1 has some indirect sub-type changes:
+    parameter 1 of type 'enum Enum2' has sub-type changes:
+      type size hasn't changed
+      1 enumerator insertion:
+        'Enum2::ENUM2_E1' value '1'
+      1 enumerator change:
+        'Enum2::LAST_ENUM1_ENUMERATOR' from value '1' to '2' at test40.1-enumerator-changes-enumerator-changes-v1.c:20:1
+
diff --git a/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-1.txt b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-1.txt
new file mode 100644
index 00000000..ff415ac5
--- /dev/null
+++ b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-1.txt
@@ -0,0 +1,21 @@
+Functions changes summary: 0 Removed, 2 Changed (1 filtered out), 0 Added functions
+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
+
+2 functions with some indirect sub-type change:
+
+  [C] 'function void fun1(Enum1)' at test40.1-enumerator-changes-enumerator-changes-v0.c:30:1 has some indirect sub-type changes:
+    parameter 1 of type 'enum Enum1' has sub-type changes:
+      type size hasn't changed
+      1 enumerator insertion:
+        'Enum1::ENUM1_E1' value '1'
+      1 enumerator change:
+        'Enum1::ENUM1_MAX' from value '1' to '2' at test40.1-enumerator-changes-enumerator-changes-v1.c:13:1
+
+  [C] 'function void fun2(Enum2)' at test40.1-enumerator-changes-enumerator-changes-v0.c:35:1 has some indirect sub-type changes:
+    parameter 1 of type 'enum Enum2' has sub-type changes:
+      type size hasn't changed
+      1 enumerator insertion:
+        'Enum2::ENUM2_E1' value '1'
+      1 enumerator change:
+        'Enum2::LAST_ENUM1_ENUMERATOR' from value '1' to '2' at test40.1-enumerator-changes-enumerator-changes-v1.c:20:1
+
diff --git a/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-2.txt b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-2.txt
new file mode 100644
index 00000000..7e4093ca
--- /dev/null
+++ b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-2.txt
@@ -0,0 +1,21 @@
+Functions changes summary: 0 Removed, 2 Changed (1 filtered out), 0 Added functions
+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
+
+2 functions with some indirect sub-type change:
+
+  [C] 'function void fun0(Enum0)' at test40.1-enumerator-changes-enumerator-changes-v0.c:25:1 has some indirect sub-type changes:
+    parameter 1 of type 'enum Enum0' has sub-type changes:
+      type size hasn't changed
+      1 enumerator insertion:
+        'Enum0::ENUM0_E1' value '1'
+      1 enumerator change:
+        'Enum0::ENOM0_LAST' from value '1' to '2' at test40.1-enumerator-changes-enumerator-changes-v1.c:6:1
+
+  [C] 'function void fun2(Enum2)' at test40.1-enumerator-changes-enumerator-changes-v0.c:35:1 has some indirect sub-type changes:
+    parameter 1 of type 'enum Enum2' has sub-type changes:
+      type size hasn't changed
+      1 enumerator insertion:
+        'Enum2::ENUM2_E1' value '1'
+      1 enumerator change:
+        'Enum2::LAST_ENUM1_ENUMERATOR' from value '1' to '2' at test40.1-enumerator-changes-enumerator-changes-v1.c:20:1
+
diff --git a/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-3.txt b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-3.txt
new file mode 100644
index 00000000..9f4bbcb2
--- /dev/null
+++ b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-3.txt
@@ -0,0 +1,13 @@
+Functions changes summary: 0 Removed, 1 Changed (2 filtered out), 0 Added functions
+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
+
+1 function with some indirect sub-type change:
+
+  [C] 'function void fun2(Enum2)' at test40.1-enumerator-changes-enumerator-changes-v0.c:35:1 has some indirect sub-type changes:
+    parameter 1 of type 'enum Enum2' has sub-type changes:
+      type size hasn't changed
+      1 enumerator insertion:
+        'Enum2::ENUM2_E1' value '1'
+      1 enumerator change:
+        'Enum2::LAST_ENUM1_ENUMERATOR' from value '1' to '2' at test40.1-enumerator-changes-enumerator-changes-v1.c:20:1
+
diff --git a/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-4.txt b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-4.txt
new file mode 100644
index 00000000..e09641c6
--- /dev/null
+++ b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-4.txt
@@ -0,0 +1,21 @@
+Functions changes summary: 0 Removed, 2 Changed (1 filtered out), 0 Added functions
+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
+
+2 functions with some indirect sub-type change:
+
+  [C] 'function void fun0(Enum0)' at test40.1-enumerator-changes-enumerator-changes-v0.c:25:1 has some indirect sub-type changes:
+    parameter 1 of type 'enum Enum0' has sub-type changes:
+      type size hasn't changed
+      1 enumerator insertion:
+        'Enum0::ENUM0_E1' value '1'
+      1 enumerator change:
+        'Enum0::ENOM0_LAST' from value '1' to '2' at test40.1-enumerator-changes-enumerator-changes-v1.c:6:1
+
+  [C] 'function void fun1(Enum1)' at test40.1-enumerator-changes-enumerator-changes-v0.c:30:1 has some indirect sub-type changes:
+    parameter 1 of type 'enum Enum1' has sub-type changes:
+      type size hasn't changed
+      1 enumerator insertion:
+        'Enum1::ENUM1_E1' value '1'
+      1 enumerator change:
+        'Enum1::ENUM1_MAX' from value '1' to '2' at test40.1-enumerator-changes-enumerator-changes-v1.c:13:1
+
diff --git a/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-5.txt b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-5.txt
new file mode 100644
index 00000000..97239c74
--- /dev/null
+++ b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-5.txt
@@ -0,0 +1,3 @@
+Functions changes summary: 0 Removed, 0 Changed (3 filtered out), 0 Added functions
+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
+
diff --git a/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v0.c b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v0.c
new file mode 100644
index 00000000..7b32ee8b
--- /dev/null
+++ b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v0.c
@@ -0,0 +1,37 @@
+/**
+ * compile this with:
+ *   gcc -g -c test40.1-enumerator-changes-enumerator-changes-v0.c
+ */
+
+enum Enum0
+{
+  ENUM0_E0,
+  ENOM0_LAST
+};
+
+enum Enum1
+{
+  ENUM1_E0,
+  ENUM1_MAX
+};
+
+enum Enum2
+{
+  ENUM2_E0,
+  LAST_ENUM1_ENUMERATOR
+};
+
+void
+fun0(enum Enum0 p __attribute__((unused)))
+{
+}
+
+void
+fun1(enum Enum1 p __attribute__((unused)))
+{
+}
+
+void
+fun2(enum Enum2 p __attribute__((unused)))
+{
+}
diff --git a/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v0.o b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v0.o
new file mode 100644
index 0000000000000000000000000000000000000000..8234f05d0e157a66a88be438cbe4a1f17c2fa01f
GIT binary patch
literal 3944
zcmc&$&2Jl35TEthaf%b?t3XUj<4{o2s$PGP21rTbD3~T9Ng*^65+Cl`UVl(~ZQ1K6
z1OYu26*#mKhaNa0AtVGvAWod%!e0QKI8li!M+69FcIT}(&l?Hl!dUxe<~JX2-rIR^
z{Z4N4*?^*eAO)78rwJ6GdS6e@+F}+?!UXKD-@JG8t-o%)`n{j@2YQA;8Ki&^4G+`c
zrPCfIK=KAY12i~Q*rNgi!7v$JkD+U_hie+YA)A5}dWHrfu@L2>&rwrbaE5XPZr8!~
za|`PE{pk3eexCj8Qk0mxO+8P{7Sq@ZlsQnNry`g;sHoB9nQ&w#xG}Lfqg;$UH8Y@0
z;N&4t2N@XKm<UV)1T|$PrbZc5W@BT@csM(G5F`K4nDSV3BBD-DO%pu&Aa2#85Q!{D
z)F?ohLnD|!(hJ0L&QnSF0N8KBmS}L->LQM?i7bS^=a4Ik@@I6I7_Ao{6sGN^2bAf+
zh?CY13n_5IRn?f%&EhYhVnA;D@>VLpx%%Q2=$f5c#WYIsnrXqM?cMlV`iXctt<7uc
zcqWy3JhhOz5TDyYhjrazLn3oFo@iKI(^#q)rqQkyQPtbU>eBv`3yFpK#9k(zs6cLe
zcPo?6r66Z^8#tnxvQ-q)Jz;D0C7@FB<bXJixt-N3mv^AtH8V7%X>ih{?PM89R+|kY
zS!$MEuO%xrD_O4<^g^wo*XqfZZWXIWCutcStCKA0mTrrQQmtH0bh@op8^|=D($Wb7
zD>K@<)odq<Ro$!@9Y1I<r4|2&8IMyXTv}UOjL+eVJo_KsBDTp%yH&7@4t7A<A62G?
z!h^T4olYT650QPIR->ag0&BwqIdnjbY)_LB=_vavj@j4FranfU9E9Kh=`$dJt?LIP
z?zZ_MyNVV+Qk|Qn&!oaYA4Y%*_6^f#Q{hA(=J?|!<3lP8`Jj{^Zwnt%VZ;X||KUFT
z0wUv{{SkNIBI80Ae1&oG{sn%lvtb`kaZodMZQ^31yxv_El@m_8hgYmk<ID-CXPNZ}
zB%D=vhz05h@7q7;@fS4^l=R><Z<F;U3GcGLCgH3CUByJAe(|PXa9|hzhV{IJzs<TK
z;qS6um+)^{KalV;>(?dxHtQmvsPhxXKVUBQO}zgvq&%Oo{<Y*!`G|gUn7iIDS^tH(
z$j_?V0~Bxn^I*g15&n-bPX6??SwAn~cUezM_^+&AWG>eeXPkP++do+e7wf$$;bPxF
z>%7sh^a66L?eMCQEMwmSjlMvj6*?Wz+D2X1N=Bhu$=A$s6K%Gv7Yc1-?~qw<+h3zY
zSiNQ%A_S)pBEflEFE$$u13%-M*|ZD|zX6Gkr5CSp5~G?gxAlf-JYLUn*%SuSo_fzV
z<;v24B>$BmUoXv|LYRC*zvgxF$a4HF+UR^AH=g=M&PmgW`Tr)xL$WXRA^ktidVEOy
zfj4B7n`#U(@)LKFxbNuRk#iyhFEjWaDISu2>A$axk^4OcQ=V87W#8uz^Q-@7E>y%*
z{b79M`b$1J5rX@9-Bh2K<oW4d^qYT>KdDzanC2%)p1*|{<rH<IXp_P7e<uLuVt&Ej
zMBFcamK$nPI35xI{t@vjv?wI;y>YbO{)@C9qCv>Qhq%e{UW{bf_fy7rxvqPQ<M&;}
z5ybup_+u2vH7fw-Ro?$~)@lDo@DcH%f2kgMZVLI0=fA@R(|4Wz-N^AYjeK?yg6n*K
Yo*~6U^7^SRzxcac|5u(^66N@R0n{#JCIA2c

literal 0
HcmV?d00001

diff --git a/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v1.c b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v1.c
new file mode 100644
index 00000000..e7f1bcc7
--- /dev/null
+++ b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v1.c
@@ -0,0 +1,40 @@
+/**
+ * compile this with:
+ *   gcc -g -c test40.1-enumerator-changes-enumerator-changes-v1.c
+ */
+
+enum Enum0
+{
+  ENUM0_E0,
+  ENUM0_E1,
+  ENOM0_LAST
+};
+
+enum Enum1
+{
+  ENUM1_E0,
+  ENUM1_E1,
+  ENUM1_MAX
+};
+
+enum Enum2
+{
+  ENUM2_E0,
+  ENUM2_E1,
+  LAST_ENUM1_ENUMERATOR
+};
+
+void
+fun0(enum Enum0 p __attribute__((unused)))
+{
+}
+
+void
+fun1(enum Enum1 p __attribute__((unused)))
+{
+}
+
+void
+fun2(enum Enum2 p __attribute__((unused)))
+{
+}
diff --git a/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v1.o b/tests/data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v1.o
new file mode 100644
index 0000000000000000000000000000000000000000..5438d75e9486777259484d9feec98fba7ff0f3f2
GIT binary patch
literal 4064
zcmc&$&2Jl35P$1+oD?@s(o~R;Kx_m}TGi_h(f}z*9R<@w<f94+^Z>YPdt;mW3wxcW
zMF_;9qQIf8IFt_|B)A}PsKh_OhXfZcoH!x%0218b1QJLv^LEDGJZ~hF3uEnjGr#%G
zyxI5VeKo!Eq)*cXizenp$6{27J9l;TlvGpVs2CPoOV@tB_R8Papa0rR3qq<Mq4i-v
z4>tE<<Cgnfh7a|1cmmisRmj$<&nE)@UZk{n{g=FXZNkx8-UL6UIfe}iznb>J7hvgi
zaSZeNZj^=OMZl2K;r#+O@FLGZk33nHHtv)RSceI$af4Vwn-Y3xJP0v<O%KgSdV>-F
z^6+d#I~{y163~VrX^+tR2pC!(_KgVPH?%XOdWb-6a&$;L(wiE&GK&13A?=~ia8REZ
zn?QK*9+<obMKCxY)I&n_QfLtJ2RngK&Pgl@9wFq~NDUkJa<oBER^aIY?g{8MP5Ud<
zj~%`*JfKX-SNCZXz5ypKcpyR(huWq-uC-F|^XU+fURz&{XI2)T-W08Bvs9>B`EaRf
zi}c#oY9h0`a9*5U+X^ox9tkHB#<Y<LC*#S7<1_J7;i(OvE}1qtqRHdoXvJ<-t+|3#
zwHl=ym}VnaoZEeLCOR`6-ARU{1<IR_OC^~>rP``EW?Y&uFenLTItnlo0;|k8g<+fC
zSlC?O5ZkS45}O1zPCAHzq)8Dg)+$ykU(3H(iWN$BtX#^P*;2tQm1A|&&K0d@%(j|#
zGnO}PQ>xK?X?r``Y}M-xfv)Mek%(H5%xai+tr5)?&1%7FdO<r0Blkayc#tZ^*~P`#
z@D!}<@&B-_atFa3<A`=;Luk8$+SuXVzU#2r#sS6$MV_{W(BM_yVt*hF1i;YuI66W0
z^FQafJ^keB>TsX?#qa-gIRJo-?F9qgJ_LLGI(WQ9ckZ1omo5Tb7y`O;|8)6uai|M(
z_;3Yzh%OF$pqL--5)aYEfCq~F{x19!AV*yD!#CtC;cRn%hHzeg#t(En<O$`onv}Ki
zwTF4#v&xktj<ffGWMl1)Adb&G*>_XVIGN&JQgBDOSO0{I&ov+vbKyAd>txR<_#0#|
zD)<eumld2$v7z9+1D;jz@5#0lyiWE-1%Hq1mlPcSxTNeV_zz@XRq)GXzpdaOlYLXc
zKO_5H1^<fd52@$*=Y900!vB`+ZxlYRC(5r*C)XU;<vX%}qn`7V+1^VSFTZ%mF}M$(
z_ZRP<2Juc(&-iV!6AJzl*{7*j>j@K%dk`O%6e*1J`(99Ro;P7MFIQ|c3%%WN`XXz#
zwQCCl|ICDuZ8n9`u*#;9x3aB5rc~XoflumYHrudv_PEW4{A1gLl}lBNLtqG25za_E
zSF2Pk_*gKiHQO@a?<Cr^&D=#wVihyn4YR@<nBM20-53PooVxD_%$33hMEzhwu3I8t
zAg12*FStoAU5%dvAKnuOjmQ0>=EUK6`~k(OnDRprzk*X3<dQ#WLeA5s8Ui2k_)g>V
zkN20F6Vp@(d=m|qsN8elQ!%9fL15e!i>k_f3NWww&*5PP;drbczv9*U^MI*20TPq+
z-dLYoRBi^GUgN(;<6od)93LTd{5oKmlj{Uk$|V~AM+T_p@wtB)aIg4H+E61*+$a96
zed5dbQXu@l<yf7$;<WF8gLUR$|31aLG1OJ=r-ae#w&@>HJpaG2gXfR!Pe4IxiUI2H
z(EKlvjq{IRGaS$R7wb{S#*h)Z+;36A_<ew1NoqU}qn=%W#BDl1PoUuv_5HCfulOmt
Mus?UjqN>LK3)#(YJ^%m!

literal 0
HcmV?d00001

diff --git a/tests/test-diff-suppr.cc b/tests/test-diff-suppr.cc
index 11be9b9a..46f29242 100644
--- a/tests/test-diff-suppr.cc
+++ b/tests/test-diff-suppr.cc
@@ -1836,6 +1836,66 @@ InOutSpec in_out_specs[] =
     "data/test-diff-suppr/test40-enumerator-changes-report-0.txt",
     "output/test-diff-suppr/test40-enumerator-changes-report-0.txt"
   },
+  {
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v0.o",
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v1.o",
+    "",
+    "",
+    "",
+    "--no-default-suppression",
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-0.txt",
+    "output/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-0.txt"
+  },
+  {
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v0.o",
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v1.o",
+    "",
+    "",
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-1.suppr",
+    "--no-default-suppression",
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-1.txt",
+    "output/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-1.txt"
+  },
+  {
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v0.o",
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v1.o",
+    "",
+    "",
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-2.suppr",
+    "--no-default-suppression",
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-2.txt",
+    "output/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-2.txt"
+  },
+  {
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v0.o",
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v1.o",
+    "",
+    "",
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-3.suppr",
+    "--no-default-suppression",
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-3.txt",
+    "output/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-3.txt"
+  },
+  {
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v0.o",
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v1.o",
+    "",
+    "",
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-4.suppr",
+    "--no-default-suppression",
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-4.txt",
+    "output/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-4.txt"
+  },
+  {
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v0.o",
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-v1.o",
+    "",
+    "",
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-5.suppr",
+    "--no-default-suppression",
+    "data/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-5.txt",
+    "output/test-diff-suppr/test40.1-enumerator-changes-enumerator-changes-report-5.txt"
+  },
   {
     "data/test-diff-suppr/libtest41-enumerator-changes-v0.so",
     "data/test-diff-suppr/libtest41-enumerator-changes-v1.so",

John Moon Oct. 2, 2023, 7:23 p.m. UTC | #2

On 9/29/2023 11:56 PM, Dodji Seketeli wrote:

> 
> As the property you've added to the suppression specification
> "language" is "changed_enumerators_regexp", (note the 's' at the end
> of enumerator), I'd say that the name of these accessor member
> functions you are declaring should rather be
> 'get_changed_enumerators_regexp' and 'set_changed_enumerators_regexp'
> (note the 's' at the end enumerator here as well), for the sake of
> consistency.

Agreed. Wasn't sure where the plural should go and I guess it got lost! 
I'm fine with whatever naming you'd like.

>> @@ -2154,6 +2186,34 @@ read_type_suppression(const ini::config::section& section)
>>   	changed_enumerator_names.push_back(p->get_value()->as_string());
>>       }
>>
>> +  /// Support 'changed_enumerators_regexp = .*_foo, bar_[0-9]+, baz'
>> +  ///
>> +  /// If the current type is an enum and if it carries changed
>> +  /// enumerators that match regular expressions listed in the
>> +  /// changed_enumerator_regexp property value then it should be
>> +  /// suppressed.
>> +
>> +  ini::property_sptr changed_enumerators_regexp_prop =
>> +    section.find_property("changed_enumerators_regexp");
>> +
>> +  vector<regex_t_sptr> changed_enumerator_regexp;
>> +  if (changed_enumerators_regexp_prop)
>> +    {
>> +      if (ini::list_property_sptr p =
>> +	  is_list_property(changed_enumerators_regexp_prop))
>> +      {
>> +	for (string e : p->get_value()->get_content())
>> +	  changed_enumerator_regexp.push_back(regex::compile(e));
>> +      }
> 
> Nice.
> 
> Please note that in earlier cases where we expect regular expressions,
> we store the regex *string* in a data member of
> type_suppression::priv, before compiling it.  Here, you compile the
> string into a regex directly.  It's fine too.  If at some point in the
> future we need to regex string, we'll store it.
> 

Okay - we'll leave as is for now.

>> +      else if (ini::simple_property_sptr p =
>> +	       is_simple_property(changed_enumerators_prop))
> 
> I think there is a (copy-&-paste) typo here.  It should probably not
> be 'changed_enumerators_prop', but rather
> 'changed_enumerators_regexp_prop'.  This is to support the case where
> there is just one regular expression (as opposed to a list) provided
> as the value of the property.

Good catch! I didn't test the single regex case - so glad you saw it.

> So, long story short, I have amended your patch to add the
> documentation to doc/manuals/libabigail-concepts.rst, tests to the
> testsuite and I have also added a ChangeLog part to the commit log and
> apply the changes I suggested during this review.
> 
> Please find below the resulting patch.  Maybe you could test it in your
> environment and review it again, to catch potential issues I could
> have introduced.
> 
> In any case, thank you very much for caring so much.

Thank you for going ahead with the changes! I'll be mindful in future 
patches to check all the boxes you laid out above.

> diff --git a/doc/manuals/libabigail-concepts.rst b/doc/manuals/libabigail-concepts.rst
> index 4f26932c..225d61eb 100644
> --- a/doc/manuals/libabigail-concepts.rst
> +++ b/doc/manuals/libabigail-concepts.rst
> @@ -730,6 +730,30 @@ changes, unless the ``has_size_changes`` property is set to ``yes``.
>   
>         changed_enumerators = LAST_ENUMERATORS0, LAST_ENUMERATOR1
>   
> +.. _suppr_changed_enumerators_regexp_label:
> +
> +* ``changed_enumerators_regexp``
> +
> +  Usage:
> +
> +    ``changed_enumerators_regexp`` ``=`` <list-of-enumerator-regular-expressions>
> +

Nit: in git, it shows a trailing whitespace here. Not sure if this 
project cares about that.

The rest of the adjustments you made look great to me! Again, thanks so 
much for writing the tests and documentation. It's very much appreciated.

Let me know if you'd like me to send a v2 of the patch with your changes 
or you can apply directly to the WIP branch.

Thanks,
John

Dodji Seketeli Oct. 2, 2023, 9:42 p.m. UTC | #3

Hello,

[...]

On 9/29/2023 11:56 PM, Dodji Seketeli wrote:

>> So, long story short, I have amended your patch to add the
>> documentation to doc/manuals/libabigail-concepts.rst, tests to the
>> testsuite and I have also added a ChangeLog part to the commit log and
>> apply the changes I suggested during this review.
>> Please find below the resulting patch.  Maybe you could test it in
>> your
>> environment and review it again, to catch potential issues I could
>> have introduced.
>> In any case, thank you very much for caring so much.

John Moon <quic_johmoo@quicinc.com> a écrit:

>
> Thank you for going ahead with the changes! I'll be mindful in future
> patches to check all the boxes you laid out above.

No problem.  These bits are somewhat lateral to the central piece you
(rightfully) concentrated on.  The bits I pushed are just mechanical.

>> diff --git a/doc/manuals/libabigail-concepts.rst b/doc/manuals/libabigail-concepts.rst
>> index 4f26932c..225d61eb 100644
>> --- a/doc/manuals/libabigail-concepts.rst
>> +++ b/doc/manuals/libabigail-concepts.rst
>> @@ -730,6 +730,30 @@ changes, unless the ``has_size_changes`` property is set to ``yes``.
>>           changed_enumerators = LAST_ENUMERATORS0, LAST_ENUMERATOR1
>>   +.. _suppr_changed_enumerators_regexp_label:
>> +
>> +* ``changed_enumerators_regexp``
>> +
>> +  Usage:
>> +
>> +    ``changed_enumerators_regexp`` ``=`` <list-of-enumerator-regular-expressions>
>> +
>
> Nit: in git, it shows a trailing whitespace here. Not sure if this
> project cares about that.

Right.  I fixed it in the patch, thanks for catching it.  I realized
there are similar pre-existing trailing white spaces after the line:

    ``changed_enumerators`` ``=`` LAST_ENUMERATORS0, LAST_ENUMERATOR1

I have actually copied the later to write the former.  I have removed
the trailing white spaces there too in a separate patch in the master
branch posted at
https://inbox.sourceware.org/libabigail/87v8bohfw5.fsf@redhat.com.

> The rest of the adjustments you made look great to me! Again, thanks
> so much for writing the tests and documentation. It's very much
> appreciated.

It's my pleasure.

> Let me know if you'd like me to send a v2 of the patch with your
> changes or you can apply directly to the WIP branch.

Nah, I'll just go ahead and apply the resulting patch below to the WIP
branch 'check-uapi-support' browsable at
https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/check-uapi-support.

[...]

Cheers,

diff mbox series

Patch

diff --git a/include/abg-suppression.h b/include/abg-suppression.h
index 9449d06d..8fc6516d 100644
--- a/include/abg-suppression.h
+++ b/include/abg-suppression.h
@@ -330,6 +330,12 @@  public:
   void
   set_changed_enumerator_names(const vector<string>&);

+  const vector<regex::regex_t_sptr>&
+  get_changed_enumerator_regexp() const;
+
+  void
+  set_changed_enumerator_regexp(const vector<regex::regex_t_sptr>&);
+
   virtual bool
   suppresses_diff(const diff* diff) const;

diff --git a/src/abg-suppression-priv.h b/src/abg-suppression-priv.h
index cf66a9f1..cc7b4b26 100644
--- a/src/abg-suppression-priv.h
+++ b/src/abg-suppression-priv.h
@@ -585,6 +585,7 @@  class type_suppression::priv
   string				source_location_to_keep_regex_str_;
   mutable regex::regex_t_sptr		source_location_to_keep_regex_;
   mutable vector<string>		changed_enumerator_names_;
+  mutable vector<regex::regex_t_sptr>	changed_enumerator_regexp_;

   priv();

diff --git a/src/abg-suppression.cc b/src/abg-suppression.cc
index 4ed42e82..345b6e6f 100644
--- a/src/abg-suppression.cc
+++ b/src/abg-suppression.cc
@@ -770,6 +770,28 @@  void
 type_suppression::set_changed_enumerator_names(const vector<string>& n)
 {priv_->changed_enumerator_names_ = n;}

+/// Getter of the vector of the regular expression strings for changed
+/// enumerators that are supposed to be suppressed. Note that this
+/// will be "valid" only if the type suppression has the
+/// 'type_kind = enum' property.
+///
+/// @return the vector of the regular expression strings that are
+/// supposed to match enumertor names to be suppressed.
+const vector<regex::regex_t_sptr>&
+type_suppression::get_changed_enumerator_regexp() const
+{return priv_->changed_enumerator_regexp_;}
+
+/// Setter of the vector of the regular expression strings for changed
+/// enumerators that are supposed to be suppressed. Note that this
+/// will be "valid" only if the type suppression has the
+/// 'type_kind = enum' property.
+///
+/// @param n the vector of the regular expression strings that are
+/// supposed to match enumertor names to be suppressed.
+void
+type_suppression::set_changed_enumerator_regexp(const vector<regex::regex_t_sptr>& n)
+{priv_->changed_enumerator_regexp_ = n;}
+
 /// Evaluate this suppression specification on a given diff node and
 /// say if the diff node should be suppressed or not.
 ///
@@ -996,9 +1018,12 @@  type_suppression::suppresses_diff(const diff* diff) const
       // ... and yet carries some changed enumerators!
       && !enum_dif->changed_enumerators().empty())
     {
-      // Make sure that all changed enumerators are listed in the
-      // vector of enumerator names returned by the
-      // get_changed_enumerator_names() member function.
+
+      // Make sure that all changed enumerators are either:
+      //  1. listed in the vector of enumerator names returned
+      //     by the get_changed_enumerator_names() member function
+      //  2. match a regular expression returned by the
+      //     get_changed_enumerator_regexp() member function
       bool matched = true;
       for (string_changed_enumerator_map::const_iterator i =
 	     enum_dif->changed_enumerators().begin();
@@ -1006,13 +1031,20 @@  type_suppression::suppresses_diff(const diff* diff) const
 	   ++i)
 	{
 	  matched &= true;
-	  if (std::find(get_changed_enumerator_names().begin(),
-			get_changed_enumerator_names().end(),
-			i->first) == get_changed_enumerator_names().end())
-	    {
-	      matched &= false;
-	      break;
-	    }
+	  if ((std::find(get_changed_enumerator_names().begin(),
+			 get_changed_enumerator_names().end(),
+			 i->first) == get_changed_enumerator_names().end())
+	     &&
+	      (std::find_if(get_changed_enumerator_regexp().begin(),
+			    get_changed_enumerator_regexp().end(),
+			    [&] (const regex_t_sptr& enum_regexp)
+		{
+		  return regex::match(enum_regexp, i->first);
+		}) == get_changed_enumerator_regexp().end()))
+	  {
+	    matched &= false;
+	    break;
+	  }
 	}
       if (!matched)
 	return false;
@@ -2154,6 +2186,34 @@  read_type_suppression(const ini::config::section& section)
 	changed_enumerator_names.push_back(p->get_value()->as_string());
     }

+  /// Support 'changed_enumerators_regexp = .*_foo, bar_[0-9]+, baz'
+  ///
+  /// If the current type is an enum and if it carries changed
+  /// enumerators that match regular expressions listed in the
+  /// changed_enumerator_regexp property value then it should be
+  /// suppressed.
+
+  ini::property_sptr changed_enumerators_regexp_prop =
+    section.find_property("changed_enumerators_regexp");
+
+  vector<regex_t_sptr> changed_enumerator_regexp;
+  if (changed_enumerators_regexp_prop)
+    {
+      if (ini::list_property_sptr p =
+	  is_list_property(changed_enumerators_regexp_prop))
+      {
+	for (string e : p->get_value()->get_content())
+	  changed_enumerator_regexp.push_back(regex::compile(e));
+      }
+      else if (ini::simple_property_sptr p =
+	       is_simple_property(changed_enumerators_prop))
+      {
+	changed_enumerator_regexp.push_back(
+	  regex::compile(p->get_value()->as_string())
+	);
+      }
+    }
+
   if (section.get_name() == "suppress_type")
     result.reset(new type_suppression(label_str, name_regex_str, name_str));
   else if (section.get_name() == "allow_type")
@@ -2217,6 +2277,10 @@  read_type_suppression(const ini::config::section& section)
       && !changed_enumerator_names.empty())
     result->set_changed_enumerator_names(changed_enumerator_names);

+  if (result->get_type_kind() == type_suppression::ENUM_TYPE_KIND
+      && !changed_enumerator_regexp.empty())
+    result->set_changed_enumerator_regexp(changed_enumerator_regexp);
+
   return result;
 }