[1/2] abidiff: More compact references to prior diffs.

  In both the default and leaf reporting modes, when there is a repeated
or circular reference to a type difference of a member variable, some
placeholder text is emitted instead.

In the leaf reporter:

  type 'struct S' of 'foo::bar' changed as reported earlier

In the default reporter, this spans two lines:

  type of 'S foo::bar' changed:
    details were reported earlier

This patch changes the latter to the more compact:

  type of 'S foo::bar' changed, as reported earlier

More generally, this patch makes the punctuation of such placeholder
text more consistent with a comma separating the phrases in all cases.

It doesn't attempt to reconcile the different formatting of member
variable declarations between the two modes.

	* src/abg-reporter-priv.cc (represent): In the var_diff_sptr
	overload, use consistent punctuation and keep to a single line
	of output when referring back to an existing type diff report.
	* src/abg-reporter-priv.h: In the macro
	RETURN_IF_BEING_REPORTED_OR_WAS_REPORTED_EARLIER, use
	consistent punctuation when referring back to an existing type
	diff report.
	* tests/data/test-abidiff/test-PR18791-report0.txt: Adjust
	formatting of back references to existing type diff reports.
	* tests/data/test-diff-dwarf/PR25058-liblttng-ctl-report-1.txt:
	Ditto.
	* tests/data/test-diff-filter/test16-report-2.txt: Ditto.
	* tests/data/test-diff-filter/test17-1-report.txt: Ditto.
	* tests/data/test-diff-filter/test25-cyclic-type-report-1.txt:
	Ditto.
	* tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt:
	Ditto.
	* tests/data/test-diff-suppr/test36-leaf-report-0.txt: Ditto.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 src/abg-reporter-priv.cc                      | 19 ++++++++-----
 src/abg-reporter-priv.h                       |  8 +++---
 .../test-abidiff/test-PR18791-report0.txt     |  3 +--
 .../PR25058-liblttng-ctl-report-1.txt         | 22 +++++++--------
 .../data/test-diff-filter/test16-report-2.txt |  2 +-
 .../data/test-diff-filter/test17-1-report.txt |  2 +-
 .../test25-cyclic-type-report-1.txt           |  2 +-
 ...l7.x86_64-0.12.8-1.el7.x86_64-report-2.txt | 27 +++++++++----------
 .../test-diff-suppr/test36-leaf-report-0.txt  |  2 +-
 9 files changed, 45 insertions(+), 42 deletions(-)
  

On Fri, Apr 03, 2020 at 10:53:54PM +0100, Android Kernel Team wrote:
>In both the default and leaf reporting modes, when there is a repeated
>or circular reference to a type difference of a member variable, some
>placeholder text is emitted instead.
>
>In the leaf reporter:
>
>  type 'struct S' of 'foo::bar' changed as reported earlier
>
>In the default reporter, this spans two lines:
>
>  type of 'S foo::bar' changed:
>    details were reported earlier
>
>This patch changes the latter to the more compact:
>
>  type of 'S foo::bar' changed, as reported earlier
>
>More generally, this patch makes the punctuation of such placeholder
>text more consistent with a comma separating the phrases in all cases.
>
>It doesn't attempt to reconcile the different formatting of member
>variable declarations between the two modes.
>
>	* src/abg-reporter-priv.cc (represent): In the var_diff_sptr
>	overload, use consistent punctuation and keep to a single line
>	of output when referring back to an existing type diff report.
>	* src/abg-reporter-priv.h: In the macro
>	RETURN_IF_BEING_REPORTED_OR_WAS_REPORTED_EARLIER, use
>	consistent punctuation when referring back to an existing type
>	diff report.
>	* tests/data/test-abidiff/test-PR18791-report0.txt: Adjust
>	formatting of back references to existing type diff reports.
>	* tests/data/test-diff-dwarf/PR25058-liblttng-ctl-report-1.txt:
>	Ditto.
>	* tests/data/test-diff-filter/test16-report-2.txt: Ditto.
>	* tests/data/test-diff-filter/test17-1-report.txt: Ditto.
>	* tests/data/test-diff-filter/test25-cyclic-type-report-1.txt:
>	Ditto.
>	* tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt:
>	Ditto.
>	* tests/data/test-diff-suppr/test36-leaf-report-0.txt: Ditto.
>
>Signed-off-by: Giuliano Procida <gprocida@google.com>
>---
> src/abg-reporter-priv.cc                      | 19 ++++++++-----
> src/abg-reporter-priv.h                       |  8 +++---
> .../test-abidiff/test-PR18791-report0.txt     |  3 +--
> .../PR25058-liblttng-ctl-report-1.txt         | 22 +++++++--------
> .../data/test-diff-filter/test16-report-2.txt |  2 +-
> .../data/test-diff-filter/test17-1-report.txt |  2 +-
> .../test25-cyclic-type-report-1.txt           |  2 +-
> ...l7.x86_64-0.12.8-1.el7.x86_64-report-2.txt | 27 +++++++++----------
> .../test-diff-suppr/test36-leaf-report-0.txt  |  2 +-
> 9 files changed, 45 insertions(+), 42 deletions(-)
>
>diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc
>index 883c815e..323de503 100644
>--- a/src/abg-reporter-priv.cc
>+++ b/src/abg-reporter-priv.cc
>@@ -507,11 +507,11 @@ represent(const var_diff_sptr	&diff,
>
> 	  if (d->currently_reporting())
> 	    {
>-	      out << " as being reported\n";
>+	      out << ", as being reported\n";

No need for the braces.

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

Cheers,
Matthias

> 	    }
> 	  else if (d->reported_once())
> 	    {
>-	      out << " as reported earlier\n";
>+	      out << ", as reported earlier\n";
> 	    }
> 	  else
> 	    {
>@@ -527,13 +527,20 @@ represent(const var_diff_sptr	&diff,
> 	  if (ctxt->get_reporter()->diff_to_be_reported(d.get()))
> 	    {
> 	      out << indent
>-		  << "type of '" << pretty_representation << "' changed:\n";
>+		  << "type of '" << pretty_representation << "' changed";
> 	      if (d->currently_reporting())
>-		out << indent << "  details are being reported\n";
>+		{
>+		  out << ", as being reported\n";
>+		}
> 	      else if (d->reported_once())
>-		out << indent << "  details were reported earlier\n";
>+		{
>+		  out << ", as reported earlier\n";
>+		}
> 	      else
>-		d->report(out, indent + "  ");
>+		{
>+		  out << ":\n";
>+		  d->report(out, indent + "  ");
>+		}
>
> 	      begin_with_and = true;
> 	      emitted = true;
>diff --git a/src/abg-reporter-priv.h b/src/abg-reporter-priv.h
>index b6c56fdb..b221fdc3 100644
>--- a/src/abg-reporter-priv.h
>+++ b/src/abg-reporter-priv.h
>@@ -68,14 +68,14 @@
> 	{								\
> 	  string _name_ = _diff_->first_subject()->get_pretty_representation(); \
> 	  if (_diff_->currently_reporting())				\
>-	    out << indent << INTRO_TEXT << " '" << _name_ << "' changed; " \
>-	      "details are being reported\n";				\
>+	    out << indent << INTRO_TEXT << " '" << _name_		\
>+		<< "' changed, as being reported\n";			\
> 	  else								\
> 	    {								\
> 	      out << indent << INTRO_TEXT << " '"			\
>-	      << _name_ << "' changed";				\
>+	      << _name_ << "' changed";					\
> 	      report_loc_info(D->first_subject(), *d.context(), out);	\
>-	      out << ", as reported earlier\n";			\
>+	      out << ", as reported earlier\n";				\
> 	    }								\
> 	  return ;							\
> 	}								\
>diff --git a/tests/data/test-abidiff/test-PR18791-report0.txt b/tests/data/test-abidiff/test-PR18791-report0.txt
>index 93faf599..5bc2d08b 100644
>--- a/tests/data/test-abidiff/test-PR18791-report0.txt
>+++ b/tests/data/test-abidiff/test-PR18791-report0.txt
>@@ -123,8 +123,7 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
>                     type name changed from 'std::_List_base<sigc::slot_base, std::allocator<sigc::slot_base> >::_List_impl' to 'std::__cxx11::_List_base<sigc::slot_base, std::allocator<sigc::slot_base> >::_List_impl'
>                     type size changed from 128 to 192 (in bits)
>                     1 data member change:
>-                      type of 'std::__detail::_List_node_base std::_List_base<sigc::slot_base, std::allocator<sigc::slot_base> >::_List_impl::_M_node' changed:
>-                        details were reported earlier
>+                      type of 'std::__detail::_List_node_base std::_List_base<sigc::slot_base, std::allocator<sigc::slot_base> >::_List_impl::_M_node' changed, as reported earlier
>                       and name of 'std::_List_base<sigc::slot_base, std::allocator<sigc::slot_base> >::_List_impl::_M_node' changed to 'std::__cxx11::_List_base<sigc::slot_base, std::allocator<sigc::slot_base> >::_List_impl::_M_node'
>                   and name of 'std::_List_base<sigc::slot_base, std::allocator<sigc::slot_base> >::_M_impl' changed to 'std::__cxx11::_List_base<sigc::slot_base, std::allocator<sigc::slot_base> >::_M_impl'
>
>diff --git a/tests/data/test-diff-dwarf/PR25058-liblttng-ctl-report-1.txt b/tests/data/test-diff-dwarf/PR25058-liblttng-ctl-report-1.txt
>index dfb63684..44ddc6d6 100644
>--- a/tests/data/test-diff-dwarf/PR25058-liblttng-ctl-report-1.txt
>+++ b/tests/data/test-diff-dwarf/PR25058-liblttng-ctl-report-1.txt
>@@ -179,8 +179,7 @@ Variables changes summary: 0 Removed, 0 Changed, 3 Added variables
>         in pointed to type 'struct lttng_event_field':
>           type size hasn't changed
>           1 data member change:
>-            type of 'lttng_event lttng_event_field::event' changed:
>-              details were reported earlier
>+            type of 'lttng_event lttng_event_field::event' changed, as reported earlier
>
>   [C] 'function int lttng_list_tracepoints(lttng_handle*, lttng_event**)' has some indirect sub-type changes:
>     parameter 2 of type 'lttng_event**' has sub-type changes:
>@@ -232,7 +231,7 @@ Variables changes summary: 0 Removed, 0 Changed, 3 Added variables
>                   type size changed from 576 to 640 (in bits)
>                   2 data member changes:
>                     type of 'filter_node* filter_node::parent' changed:
>-                      pointed to type 'struct filter_node' changed; details are being reported
>+                      pointed to type 'struct filter_node' changed, as being reported
>                     type of 'union {struct {} unknown; struct {filter_node* child;} root; struct {__anonymous_enum__ type; ast_link_type post_op; ast_link_type pre_op; union {char* string; uint64_t constant; double float_constant; char* identifier; filter_node* child;} u; filter_node* prev; filter_node* next;} expression; struct {op_type type; filter_node* lchild; filter_node* rchild;} op; struct {unary_op_type type; filter_node* child;} unary_op;} filter_node::u' changed:
>                       type size changed from 320 to 384 (in bits)
>                       4 data member changes:
>@@ -245,21 +244,20 @@ Variables changes summary: 0 Removed, 0 Changed, 3 Added variables
>                               type size hasn't changed
>                               1 enumerator insertion:
>                                 'ast_link_type::AST_LINK_BRACKET' value '3'
>-                            type of 'ast_link_type pre_op' changed:
>-                              details were reported earlier
>+                            type of 'ast_link_type pre_op' changed, as reported earlier
>                             type of 'union {char* string; uint64_t constant; double float_constant; char* identifier; filter_node* child;} u' changed:
>                               type size hasn't changed
>                               1 data member change:
>                                 type of 'filter_node* child' changed:
>-                                  pointed to type 'struct filter_node' changed; details are being reported
>+                                  pointed to type 'struct filter_node' changed, as being reported
>                               type changed from:
>                                 union {char* string; uint64_t constant; double float_constant; char* identifier; filter_node* child;}
>                               to:
>                                 union {char* string; uint64_t constant; double float_constant; char* identifier; filter_node* child;}
>                             type of 'filter_node* prev' changed:
>-                              pointed to type 'struct filter_node' changed; details are being reported
>+                              pointed to type 'struct filter_node' changed, as being reported
>                             type of 'filter_node* next' changed:
>-                              pointed to type 'struct filter_node' changed; details are being reported
>+                              pointed to type 'struct filter_node' changed, as being reported
>                         type of 'struct {op_type type; filter_node* lchild; filter_node* rchild;} op' changed:
>                           type size hasn't changed
>                           3 data member changes:
>@@ -278,14 +276,14 @@ Variables changes summary: 0 Removed, 0 Changed, 3 Added variables
>                                 'op_type::AST_OP_BIT_OR' value '11'
>                                 'op_type::AST_OP_BIT_XOR' value '12'
>                             type of 'filter_node* lchild' changed:
>-                              pointed to type 'struct filter_node' changed; details are being reported
>+                              pointed to type 'struct filter_node' changed, as being reported
>                             type of 'filter_node* rchild' changed:
>-                              pointed to type 'struct filter_node' changed; details are being reported
>+                              pointed to type 'struct filter_node' changed, as being reported
>                         type of 'struct {filter_node* child;} root' changed:
>                           type size hasn't changed
>                           1 data member change:
>                             type of 'filter_node* child' changed:
>-                              pointed to type 'struct filter_node' changed; details are being reported
>+                              pointed to type 'struct filter_node' changed, as being reported
>                         type of 'struct {unary_op_type type; filter_node* child;} unary_op' changed:
>                           type size hasn't changed
>                           2 data member changes:
>@@ -296,7 +294,7 @@ Variables changes summary: 0 Removed, 0 Changed, 3 Added variables
>                               1 enumerator insertion:
>                                 'unary_op_type::AST_UNARY_BIT_NOT' value '4'
>                             type of 'filter_node* child' changed:
>-                              pointed to type 'struct filter_node' changed; details are being reported
>+                              pointed to type 'struct filter_node' changed, as being reported
>                 'cds_list_head filter_ast::allocated_nodes' offset changed from 576 to 640 (in bits) (by +64 bits)
>
>   [C] 'function YYSTYPE* lttng_yyget_lval(yyscan_t)' has some indirect sub-type changes:
>diff --git a/tests/data/test-diff-filter/test16-report-2.txt b/tests/data/test-diff-filter/test16-report-2.txt
>index a90e85ad..f69eabdc 100644
>--- a/tests/data/test-diff-filter/test16-report-2.txt
>+++ b/tests/data/test-diff-filter/test16-report-2.txt
>@@ -11,6 +11,6 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
>           'int S::m0', at offset 0 (in bits)
>         1 data member change:
>           type of 'S* S::m2' changed:
>-            pointed to type 'struct S' changed; details are being reported
>+            pointed to type 'struct S' changed, as being reported
>           and offset changed from 0 to 64 (in bits) (by +64 bits)
>
>diff --git a/tests/data/test-diff-filter/test17-1-report.txt b/tests/data/test-diff-filter/test17-1-report.txt
>index 7c51152a..0b39c5d4 100644
>--- a/tests/data/test-diff-filter/test17-1-report.txt
>+++ b/tests/data/test-diff-filter/test17-1-report.txt
>@@ -11,7 +11,7 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
>           'int S::m0', at offset 0 (in bits)
>         1 data member change:
>           type of 'S* S::m2' changed:
>-            pointed to type 'struct S' changed; details are being reported
>+            pointed to type 'struct S' changed, as being reported
>           and offset changed from 0 to 64 (in bits) (by +64 bits)
>
>   [C] 'function void foo(S&)' has some indirect sub-type changes:
>diff --git a/tests/data/test-diff-filter/test25-cyclic-type-report-1.txt b/tests/data/test-diff-filter/test25-cyclic-type-report-1.txt
>index 6a6beeef..0215d892 100644
>--- a/tests/data/test-diff-filter/test25-cyclic-type-report-1.txt
>+++ b/tests/data/test-diff-filter/test25-cyclic-type-report-1.txt
>@@ -11,5 +11,5 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
>           'char S::m1', at offset 32 (in bits)
>         1 data member change:
>           type of 'S* S::m2' changed:
>-            pointed to type 'struct S' changed; details are being reported
>+            pointed to type 'struct S' changed, as being reported
>
>diff --git a/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt b/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt
>index 6d9beb85..58c94b7a 100644
>--- a/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt
>+++ b/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt
>@@ -37,7 +37,7 @@
>                             13 data member changes:
>                               type of 'QXLInstance* RedDispatcher::qxl' changed:
>                                 in pointed to type 'typedef QXLInstance' at spice-qxl.h:34:1:
>-                                  underlying type 'struct QXLInstance' changed; details are being reported
>+                                  underlying type 'struct QXLInstance' changed, as being reported
>                               type of 'Dispatcher RedDispatcher::dispatcher' changed:
>                                 underlying type 'struct Dispatcher' at dispatcher.h:22:1 changed:
>                                   type size changed from 960 to 1024 (in bits)
>@@ -51,7 +51,7 @@
>                               'int RedDispatcher::use_hardware_cursor' offset changed from 2240 to 2304 (in bits) (by +64 bits)
>                               type of 'RedDispatcher* RedDispatcher::next' changed:
>                                 in pointed to type 'typedef RedDispatcher' at red_worker.h:87:1:
>-                                  underlying type 'struct RedDispatcher' changed; details are being reported
>+                                  underlying type 'struct RedDispatcher' changed, as being reported
>                               and offset changed from 2304 to 2368 (in bits) (by +64 bits)
>                               'Ring RedDispatcher::async_commands' offset changed from 2368 to 2432 (in bits) (by +64 bits)
>                               'pthread_mutex_t RedDispatcher::async_lock' offset changed from 2496 to 2560 (in bits) (by +64 bits)
>@@ -185,7 +185,7 @@
>                                   1 data member change:
>                                     type of 'SpiceCharDeviceState* SpiceCharDeviceInstance::st' changed:
>                                       in pointed to type 'typedef SpiceCharDeviceState' at spice-char.h:34:1:
>-                                        underlying type 'struct SpiceCharDeviceState' changed; details are being reported
>+                                        underlying type 'struct SpiceCharDeviceState' changed, as being reported
>                             and offset changed from 896 to 960 (in bits) (by +64 bits)
>                             'int SpiceCharDeviceState::during_read_from_device' offset changed from 960 to 1024 (in bits) (by +64 bits)
>                             'int SpiceCharDeviceState::during_write_to_device' offset changed from 992 to 1056 (in bits) (by +64 bits)
>@@ -223,7 +223,7 @@
>                                               2 data member changes (2 filtered):
>                                                 type of 'RedChannel* RedChannelClient::channel' changed:
>                                                   in pointed to type 'typedef RedChannel' at red_channel.h:130:1:
>-                                                    underlying type 'struct RedChannel' changed; details are being reported
>+                                                    underlying type 'struct RedChannel' changed, as being reported
>                                                 type of 'RedsStream* RedChannelClient::stream' changed:
>                                                   in pointed to type 'typedef RedsStream' at reds_stream.h:31:1:
>                                                     underlying type 'struct RedsStream' at reds.h:68:1 changed:
>@@ -391,7 +391,7 @@
>                                       in pointed to type 'function type void (RedChannel*, RedClient*, RedsStream*, int, int, uint32_t*, int, uint32_t*)':
>                                         parameter 1 of type 'RedChannel*' has sub-type changes:
>                                           in pointed to type 'typedef RedChannel' at red_channel.h:130:1:
>-                                            underlying type 'struct RedChannel' changed; details are being reported
>+                                            underlying type 'struct RedChannel' changed, as being reported
>                                         parameter 3 of type 'RedsStream*' has sub-type changes:
>                                           pointed to type 'typedef RedsStream' changed at red_channel.h:134:1, as reported earlier
>                                   type of 'channel_client_disconnect_proc disconnect' changed:
>@@ -524,7 +524,7 @@
>                                     pointed to type 'typedef RedsStream' changed at red_channel.h:134:1, as reported earlier
>                                   type of 'SndWorker* SndChannel::worker' changed:
>                                     in pointed to type 'typedef SndWorker' at snd_worker.c:80:1:
>-                                      underlying type 'struct SndWorker' changed; details are being reported
>+                                      underlying type 'struct SndWorker' changed, as being reported
>                                   type of 'RedChannelClient* SndChannel::channel_client' changed:
>                                     pointed to type 'typedef RedChannelClient' changed at red_channel.h:136:1, as reported earlier
>                                   type of 'snd_channel_handle_message_proc SndChannel::handle_message' changed:
>@@ -532,26 +532,26 @@
>                                       in pointed to type 'function type int (SndChannel*, typedef size_t, typedef uint32_t, void*)':
>                                         parameter 1 of type 'SndChannel*' has sub-type changes:
>                                           in pointed to type 'typedef SndChannel' at snd_worker.c:74:1:
>-                                            underlying type 'struct SndChannel' changed; details are being reported
>+                                            underlying type 'struct SndChannel' changed, as being reported
>                                   type of 'snd_channel_on_message_done_proc SndChannel::on_message_done' changed:
>                                     underlying type 'void (SndChannel*)*' changed:
>                                       in pointed to type 'function type void (SndChannel*)':
>                                         parameter 1 of type 'SndChannel*' has sub-type changes:
>                                           in pointed to type 'typedef SndChannel' at snd_worker.c:74:1:
>-                                            underlying type 'struct SndChannel' changed; details are being reported
>+                                            underlying type 'struct SndChannel' changed, as being reported
>                                   type of 'snd_channel_cleanup_channel_proc SndChannel::cleanup' changed:
>                                     underlying type 'void (SndChannel*)*' changed:
>                                       in pointed to type 'function type void (SndChannel*)':
>                                         parameter 1 of type 'SndChannel*' has sub-type changes:
>                                           in pointed to type 'typedef SndChannel' at snd_worker.c:74:1:
>-                                            underlying type 'struct SndChannel' changed; details are being reported
>+                                            underlying type 'struct SndChannel' changed, as being reported
>                                 no data member change (1 filtered);
>                           type of 'SndWorker* SndWorker::next' changed:
>                             in pointed to type 'typedef SndWorker' at snd_worker.c:80:1:
>-                              underlying type 'struct SndWorker' changed; details are being reported
>+                              underlying type 'struct SndWorker' changed, as being reported
>                       type of 'SpicePlaybackInstance* SpicePlaybackState::sin' changed:
>                         in pointed to type 'typedef SpicePlaybackInstance' at spice-audio.h:33:1:
>-                          underlying type 'struct SpicePlaybackInstance' changed; details are being reported
>+                          underlying type 'struct SpicePlaybackInstance' changed, as being reported
>
>     [C] 'function void spice_server_playback_put_samples(SpicePlaybackInstance*, uint32_t*)' at snd_worker.c:1100:1 has some indirect sub-type changes:
>       parameter 1 of type 'SpicePlaybackInstance*' has sub-type changes:
>@@ -590,11 +590,10 @@
>                     1 data member insertion:
>                       'uint32_t SpiceRecordState::frequency', at offset 512 (in bits) at snd_worker.c:166:1
>                     2 data member changes:
>-                      type of 'SndWorker SpiceRecordState::worker' changed:
>-                        details were reported earlier
>+                      type of 'SndWorker SpiceRecordState::worker' changed, as reported earlier
>                       type of 'SpiceRecordInstance* SpiceRecordState::sin' changed:
>                         in pointed to type 'typedef SpiceRecordInstance' at spice-audio.h:67:1:
>-                          underlying type 'struct SpiceRecordInstance' changed; details are being reported
>+                          underlying type 'struct SpiceRecordInstance' changed, as being reported
>
>     [C] 'function void spice_server_record_set_mute(SpiceRecordInstance*, uint8_t)' at snd_worker.c:1279:1 has some indirect sub-type changes:
>       parameter 1 of type 'SpiceRecordInstance*' has sub-type changes:
>diff --git a/tests/data/test-diff-suppr/test36-leaf-report-0.txt b/tests/data/test-diff-suppr/test36-leaf-report-0.txt
>index d270740d..9caa9428 100644
>--- a/tests/data/test-diff-suppr/test36-leaf-report-0.txt
>+++ b/tests/data/test-diff-suppr/test36-leaf-report-0.txt
>@@ -15,7 +15,7 @@ Removed/Changed/Added variables summary: 0 Removed, 0 Changed, 0 Added variable
> 'struct leaf2 at test36-leaf-v0.cc:9:1' changed:
>   type size changed from 64 to 96 (in bits)
>   there are data member changes:
>-    type 'struct leaf1' of 'leaf2::member0' changed as reported earlier
>+    type 'struct leaf1' of 'leaf2::member0' changed, as reported earlier
>     and size changed from 32 to 64 (in bits) (by +32 bits)
>     'char leaf2::member1' offset changed from 32 to 64 (in bits) (by +32 bits)
>   3 impacted interfaces:
>-- 
>2.26.0.292.g33ef6b2f38-goog
>
>
  

Hello,

I like the patch overall, I just have some nits to pick I guess :-)

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

[...]

> diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc
> index 883c815e..323de503 100644
> --- a/src/abg-reporter-priv.cc
> +++ b/src/abg-reporter-priv.cc
> @@ -507,11 +507,11 @@ represent(const var_diff_sptr	&diff,
>  
>  	  if (d->currently_reporting())
>  	    {
> -	      out << " as being reported\n";
> +	      out << ", as being reported\n";
>  	    }

In general, throughout the code we try to only use braces when
necessary.  I know this change didn't create this situation here (I must
have missed it from a previous patch or something) but could you please
remove the brace here, for the sake of consistency with the rest of the
code.

>  	  else if (d->reported_once())
>  	    {
> -	      out << " as reported earlier\n";
> +	      out << ", as reported earlier\n";
>  	    }

Likewise.

[...]

> @@ -527,13 +527,20 @@ represent(const var_diff_sptr	&diff,
>  	  if (ctxt->get_reporter()->diff_to_be_reported(d.get()))
>  	    {
>  	      out << indent
> -		  << "type of '" << pretty_representation << "' changed:\n";
> +		  << "type of '" << pretty_representation << "' changed";
>  	      if (d->currently_reporting())
> -		out << indent << "  details are being reported\n";
> +		{
> +		  out << ", as being reported\n";
> +		}

Likewise.
>  	      else if (d->reported_once())
> -		out << indent << "  details were reported earlier\n";
> +		{
> +		  out << ", as reported earlier\n";
> +		}

Likewise.
>  	      else
> -		d->report(out, indent + "  ");
> +		{
> +		  out << ":\n";
> +		  d->report(out, indent + "  ");
> +		}

Likewise.

[...]


Matthias Maennich <maennich@google.com> a ?crit:

[...]

>>+++ b/src/abg-reporter-priv.cc
>>@@ -507,11 +507,11 @@ represent(const var_diff_sptr	&diff,
>>
>> 	  if (d->currently_reporting())
>> 	    {
>>-	      out << " as being reported\n";
>>+	      out << ", as being reported\n";
>
> No need for the braces.

Agreed.

Thank you Matthias for the review.

Cheers,
  

All done.

v2 patches on the way.

Giuliano.

On Tue, 7 Apr 2020 at 14:47, Dodji Seketeli <dodji@seketeli.org> wrote:
>
> Hello,
>
> I like the patch overall, I just have some nits to pick I guess :-)
>
> Giuliano Procida <gprocida@google.com> a ?crit:
>
> [...]
>
> > diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc
> > index 883c815e..323de503 100644
> > --- a/src/abg-reporter-priv.cc
> > +++ b/src/abg-reporter-priv.cc
> > @@ -507,11 +507,11 @@ represent(const var_diff_sptr   &diff,
> >
> >         if (d->currently_reporting())
> >           {
> > -           out << " as being reported\n";
> > +           out << ", as being reported\n";
> >           }
>
> In general, throughout the code we try to only use braces when
> necessary.  I know this change didn't create this situation here (I must
> have missed it from a previous patch or something) but could you please
> remove the brace here, for the sake of consistency with the rest of the
> code.
>
> >         else if (d->reported_once())
> >           {
> > -           out << " as reported earlier\n";
> > +           out << ", as reported earlier\n";
> >           }
>
> Likewise.
>
> [...]
>
> > @@ -527,13 +527,20 @@ represent(const var_diff_sptr   &diff,
> >         if (ctxt->get_reporter()->diff_to_be_reported(d.get()))
> >           {
> >             out << indent
> > -               << "type of '" << pretty_representation << "' changed:\n";
> > +               << "type of '" << pretty_representation << "' changed";
> >             if (d->currently_reporting())
> > -             out << indent << "  details are being reported\n";
> > +             {
> > +               out << ", as being reported\n";
> > +             }
>
> Likewise.
> >             else if (d->reported_once())
> > -             out << indent << "  details were reported earlier\n";
> > +             {
> > +               out << ", as reported earlier\n";
> > +             }
>
> Likewise.
> >             else
> > -             d->report(out, indent + "  ");
> > +             {
> > +               out << ":\n";
> > +               d->report(out, indent + "  ");
> > +             }
>
> Likewise.
>
> [...]
>
>
> Matthias Maennich <maennich@google.com> a ?crit:
>
> [...]
>
> >>+++ b/src/abg-reporter-priv.cc
> >>@@ -507,11 +507,11 @@ represent(const var_diff_sptr   &diff,
> >>
> >>        if (d->currently_reporting())
> >>          {
> >>-           out << " as being reported\n";
> >>+           out << ", as being reported\n";
> >
> > No need for the braces.
>
> Agreed.
>
> Thank you Matthias for the review.
>
> Cheers,
>
> --
>                 Dodji