[1/2] Improve readability of represent helper function.

  This change:

    - makes local variables const where possible
    - gives local variables more descriptive names
    - factors out some more expressions as local variables
    - simplifies the logic around anonymous data members

There are no behavioural changes.

	* src/abg-reporter-priv.cc (represent): In the var_diff_sptr
	overload, rename pretty_representation to
	o_pretty_representation, introduce n_pretty_representation
	where needed and replace the duplicate tr1 and tr2 with these;
	rename all other variables foo1 and foo2 to o_foo and n_foo
	respectively, using _type instead of _t; introduce o_anon and
	n_anon and use them to make the local variable
	is_strict_anonymous_data_member_change const, make ABG_ASSERT
	in anonymous data member handling more obvious in the case
	where anonymity has changed and allow simplification of
	formatting in this case; move declarations of const local
	variables above those of the non-const, state-tracking,
	variables.

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

On Wed, Apr 08, 2020 at 01:20:43PM +0100, Giuliano Procida wrote:
>This change:
>
>    - makes local variables const where possible
>    - gives local variables more descriptive names
>    - factors out some more expressions as local variables
>    - simplifies the logic around anonymous data members
>
>There are no behavioural changes.
>
>	* src/abg-reporter-priv.cc (represent): In the var_diff_sptr
>	overload, rename pretty_representation to
>	o_pretty_representation, introduce n_pretty_representation
>	where needed and replace the duplicate tr1 and tr2 with these;
>	rename all other variables foo1 and foo2 to o_foo and n_foo
>	respectively, using _type instead of _t; introduce o_anon and
>	n_anon and use them to make the local variable
>	is_strict_anonymous_data_member_change const, make ABG_ASSERT
>	in anonymous data member handling more obvious in the case
>	where anonymity has changed and allow simplification of
>	formatting in this case; move declarations of const local
>	variables above those of the non-const, state-tracking,
>	variables.
>
>Signed-off-by: Giuliano Procida <gprocida@google.com>
>---
> src/abg-reporter-priv.cc | 117 +++++++++++++++++----------------------
> 1 file changed, 50 insertions(+), 67 deletions(-)
>
>diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc
>index 5fb5fbc5..a46c0e9d 100644
>--- a/src/abg-reporter-priv.cc
>+++ b/src/abg-reporter-priv.cc
>@@ -409,14 +409,21 @@ represent(const var_diff_sptr	&diff,
>   if (!ctxt->get_reporter()->diff_to_be_reported(diff.get()))
>     return;
>
>-  var_decl_sptr o = diff->first_var();
>-  var_decl_sptr n = diff->second_var();
>-  string name1 = o->get_qualified_name();
>-  string name2 = n->get_qualified_name();
>-  uint64_t size1 = get_var_size_in_bits(o);
>-  uint64_t size2 = get_var_size_in_bits(n);
>-  uint64_t offset1 = get_data_member_offset(o);
>-  uint64_t offset2 = get_data_member_offset(n);
>+  const var_decl_sptr o = diff->first_var();
>+  const var_decl_sptr n = diff->second_var();
>+  const bool o_anon = !!is_anonymous_data_member(o);
>+  const bool n_anon = !!is_anonymous_data_member(n);
>+  const bool is_strict_anonymous_data_member_change = o_anon && n_anon;
>+  const string o_name = o->get_qualified_name();
>+  const string n_name = n->get_qualified_name();
>+  const uint64_t o_size = get_var_size_in_bits(o);
>+  const uint64_t n_size = get_var_size_in_bits(n);
>+  const uint64_t o_offset = get_data_member_offset(o);
>+  const uint64_t n_offset = get_data_member_offset(n);
>+  const string o_pretty_representation = o->get_pretty_representation();

Any reason to not have n_pretty_representation also already defined
here?

Otherwise nice cleanup!

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

Cheers,
Matthias

>+
>+  const bool show_size_offset_changes = ctxt->get_allowed_category()
>+					& SIZE_OR_OFFSET_CHANGE_CATEGORY;
>
>   // Has the main diff text been output?
>   bool emitted = false;
>@@ -424,39 +431,31 @@ represent(const var_diff_sptr	&diff,
>   bool begin_with_and = false;
>   // Have we reported a size change already?
>   bool size_reported = false;
>-  // Are we reporting a change to an anonymous member?
>-  bool is_strict_anonymous_data_member_change = false;
>
>-  string pretty_representation = o->get_pretty_representation();
>-  bool show_size_offset_changes = ctxt->get_allowed_category()
>-				  & SIZE_OR_OFFSET_CHANGE_CATEGORY;
>-
>-  if (is_anonymous_data_member(o) && is_anonymous_data_member(n))
>+  if (is_strict_anonymous_data_member_change)
>     {
>-      is_strict_anonymous_data_member_change = true;
>-      string tr1 = o->get_pretty_representation();
>-      string tr2 = n->get_pretty_representation();
>-      type_base_sptr t1 = o->get_type(), t2 = n->get_type();
>-      if (tr1 != tr2)
>+      const string n_pretty_representation = n->get_pretty_representation();
>+      const type_base_sptr o_type = o->get_type(), n_type = n->get_type();
>+      if (o_pretty_representation != n_pretty_representation)
> 	{
> 	  show_offset_or_size(indent + "anonymous data member at offset",
>-			      offset1, *ctxt, out);
>+			      o_offset, *ctxt, out);
>
> 	  out << " changed from:\n"
>-	      << indent << "  " << tr1 << "\n"
>+	      << indent << "  " << o_pretty_representation << "\n"
> 	      << indent << "to:\n"
>-	      << indent << "  " << tr2 << "\n";
>+	      << indent << "  " << n_pretty_representation << "\n";
>
> 	  begin_with_and = true;
> 	  emitted = true;
> 	}
>-      else if (get_type_name(t1) != get_type_name(t2)
>-	       && is_decl(t1) && is_decl(t2)
>-	       && is_decl(t1)->get_is_anonymous()
>-	       && is_decl(t2)->get_is_anonymous())
>+      else if (get_type_name(o_type) != get_type_name(n_type)
>+	       && is_decl(o_type) && is_decl(n_type)
>+	       && is_decl(o_type)->get_is_anonymous()
>+	       && is_decl(n_type)->get_is_anonymous())
> 	{
> 	  out << indent << "while looking at anonymous data member '"
>-	      << tr1 << "':\n"
>+	      << o_pretty_representation << "':\n"
> 	      << indent << "the internal name of that anonymous data member"
> 			   "changed from:\n"
> 	      << indent << " " << get_type_name(o->get_type()) << "\n"
>@@ -472,36 +471,20 @@ represent(const var_diff_sptr	&diff,
>     }
>   else if (filtering::has_anonymous_data_member_change(diff))
>     {
>+      ABG_ASSERT(o_anon != n_anon);
>       // So we are looking at a non-anonymous data member change from
>       // or to anonymous data member.
>-      if (is_anonymous_data_member(o))
>-	{
>-	  string repr = "anonymous data member "
>-	    + o->get_pretty_representation()
>-	    + " at offset";
>-
>-	  show_offset_or_size(indent + repr, offset1, *ctxt, out);
>-	  out << " became data member '"
>-	      << n->get_pretty_representation()
>-	      << "'\n";
>-	}
>-      else
>-	{
>-	  ABG_ASSERT(is_anonymous_data_member(n));
>-	  string repr = "data member "
>-	    + o->get_pretty_representation()
>-	    + " at offset";
>-
>-	  show_offset_or_size(indent + repr, offset1, *ctxt, out);
>-	  out << " became anonymous data member '"
>-	      << n->get_pretty_representation()
>-	      << "'\n";
>-	}
>+      const string n_pretty_representation = n->get_pretty_representation();
>+      out << indent << (o_anon ? "anonymous " : "")
>+	  << "data member " << o_pretty_representation;
>+      show_offset_or_size(" at offset", o_offset, *ctxt, out);
>+      out << " became " << (n_anon ? "anonymous " : "")
>+	  << "data member '" << n_pretty_representation << "'\n";
>
>       begin_with_and = true;
>       emitted = true;
>     }
>-  else if (diff_sptr d = diff->type_diff())
>+  else if (const diff_sptr d = diff->type_diff())
>     {
>       if (ctxt->get_reporter()->diff_to_be_reported(d.get()))
> 	{
>@@ -511,7 +494,7 @@ represent(const var_diff_sptr	&diff,
> 		<< "' changed";
> 	  else
> 	    out << indent
>-		<< "type of '" << pretty_representation << "' changed";
>+		<< "type of '" << o_pretty_representation << "' changed";
>
> 	  if (d->currently_reporting())
> 	    out << ", as being reported\n";
>@@ -529,7 +512,7 @@ represent(const var_diff_sptr	&diff,
> 	}
>     }
>
>-  if (!filtering::has_anonymous_data_member_change(diff) && name1 != name2)
>+  if (!filtering::has_anonymous_data_member_change(diff) && o_name != n_name)
>     {
>       if (filtering::has_harmless_name_change(o, n)
> 	  && !(ctxt->get_allowed_category()
>@@ -546,7 +529,7 @@ represent(const var_diff_sptr	&diff,
> 	    out << indent;
> 	  else
> 	    out << ", ";
>-	  out << "name of '" << name1 << "' changed to '" << name2 << "'";
>+	  out << "name of '" << o_name << "' changed to '" << n_name << "'";
> 	  report_loc_info(n, *ctxt, out);
> 	  emitted = true;
> 	}
>@@ -561,7 +544,7 @@ represent(const var_diff_sptr	&diff,
> 	  begin_with_and = false;
> 	}
>       else if (!emitted)
>-	out << indent << "'" << pretty_representation << "' ";
>+	out << indent << "'" << o_pretty_representation << "' ";
>       else
> 	out << ", ";
>       if (get_data_member_is_laid_out(o))
>@@ -572,7 +555,7 @@ represent(const var_diff_sptr	&diff,
>     }
>   if (show_size_offset_changes)
>     {
>-      if (offset1 != offset2)
>+      if (o_offset != n_offset)
> 	{
> 	  if (begin_with_and)
> 	    {
>@@ -584,17 +567,17 @@ represent(const var_diff_sptr	&diff,
> 	      out << indent;
> 	      if (is_strict_anonymous_data_member_change)
> 		out << "anonymous data member ";
>-	      out << "'" << pretty_representation << "' ";
>+	      out << "'" << o_pretty_representation << "' ";
> 	    }
> 	  else
> 	    out << ", ";
>
>-	  show_numerical_change("offset", offset1, offset2, *ctxt, out);
>+	  show_numerical_change("offset", o_offset, n_offset, *ctxt, out);
> 	  maybe_show_relative_offset_change(diff, *ctxt, out);
> 	  emitted = true;
> 	}
>
>-      if (!size_reported && size1 != size2)
>+      if (!size_reported && o_size != n_size)
> 	{
> 	  if (begin_with_and)
> 	    {
>@@ -606,12 +589,12 @@ represent(const var_diff_sptr	&diff,
> 	      out << indent;
> 	      if (is_strict_anonymous_data_member_change)
> 		out << "anonymous data member ";
>-	      out << "'" << pretty_representation << "' ";
>+	      out << "'" << o_pretty_representation << "' ";
> 	    }
> 	  else
> 	    out << ", ";
>
>-	  show_numerical_change("size", size1, size2, *ctxt, out);
>+	  show_numerical_change("size", o_size, n_size, *ctxt, out);
> 	  maybe_show_relative_size_change(diff, *ctxt, out);
> 	  emitted = true;
> 	}
>@@ -624,7 +607,7 @@ represent(const var_diff_sptr	&diff,
> 	  begin_with_and = false;
> 	}
>       else if (!emitted)
>-	out << indent << "'" << pretty_representation << "' ";
>+	out << indent << "'" << o_pretty_representation << "' ";
>       else
> 	out << ", ";
>       out << "elf binding changed from " << o->get_binding()
>@@ -639,7 +622,7 @@ represent(const var_diff_sptr	&diff,
> 	  begin_with_and = false;
> 	}
>       else if (!emitted)
>-	out << indent << "'" << pretty_representation << "' ";
>+	out << indent << "'" << o_pretty_representation << "' ";
>       else
> 	out << ", ";
>       out << "visibility changed from " << o->get_visibility()
>@@ -656,7 +639,7 @@ represent(const var_diff_sptr	&diff,
> 	  begin_with_and = false;
> 	}
>       else if (!emitted)
>-	out << indent << "'" << pretty_representation << "' ";
>+	out << indent << "'" << o_pretty_representation << "' ";
>       else
> 	out << ", ";
>
>@@ -675,7 +658,7 @@ represent(const var_diff_sptr	&diff,
> 	  begin_with_and = false;
> 	}
>       else if (!emitted)
>-	out << indent << "'" << pretty_representation << "' ";
>+	out << indent << "'" << o_pretty_representation << "' ";
>       else
> 	out << ", ";
>
>@@ -691,7 +674,7 @@ represent(const var_diff_sptr	&diff,
>     ;
>   else if (!emitted)
>     // We appear to have fallen off the edge of the map.
>-    out << indent << "'" << pretty_representation
>+    out << indent << "'" << o_pretty_representation
> 	<< "' has *some* difference - please report as a bug";
>   else
>     // do nothing
>-- 
>2.26.0.110.g2183baf09c-goog
>
  

Hi.

On Wed, 8 Apr 2020 at 13:47, Matthias Maennich <maennich@google.com> wrote:
>
> On Wed, Apr 08, 2020 at 01:20:43PM +0100, Giuliano Procida wrote:
> >This change:
> >
> >    - makes local variables const where possible
> >    - gives local variables more descriptive names
> >    - factors out some more expressions as local variables
> >    - simplifies the logic around anonymous data members
> >
> >There are no behavioural changes.
> >
> >       * src/abg-reporter-priv.cc (represent): In the var_diff_sptr
> >       overload, rename pretty_representation to
> >       o_pretty_representation, introduce n_pretty_representation
> >       where needed and replace the duplicate tr1 and tr2 with these;
> >       rename all other variables foo1 and foo2 to o_foo and n_foo
> >       respectively, using _type instead of _t; introduce o_anon and
> >       n_anon and use them to make the local variable
> >       is_strict_anonymous_data_member_change const, make ABG_ASSERT
> >       in anonymous data member handling more obvious in the case
> >       where anonymity has changed and allow simplification of
> >       formatting in this case; move declarations of const local
> >       variables above those of the non-const, state-tracking,
> >       variables.
> >
> >Signed-off-by: Giuliano Procida <gprocida@google.com>
> >---
> > src/abg-reporter-priv.cc | 117 +++++++++++++++++----------------------
> > 1 file changed, 50 insertions(+), 67 deletions(-)
> >
> >diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc
> >index 5fb5fbc5..a46c0e9d 100644
> >--- a/src/abg-reporter-priv.cc
> >+++ b/src/abg-reporter-priv.cc
> >@@ -409,14 +409,21 @@ represent(const var_diff_sptr    &diff,
> >   if (!ctxt->get_reporter()->diff_to_be_reported(diff.get()))
> >     return;
> >
> >-  var_decl_sptr o = diff->first_var();
> >-  var_decl_sptr n = diff->second_var();
> >-  string name1 = o->get_qualified_name();
> >-  string name2 = n->get_qualified_name();
> >-  uint64_t size1 = get_var_size_in_bits(o);
> >-  uint64_t size2 = get_var_size_in_bits(n);
> >-  uint64_t offset1 = get_data_member_offset(o);
> >-  uint64_t offset2 = get_data_member_offset(n);
> >+  const var_decl_sptr o = diff->first_var();
> >+  const var_decl_sptr n = diff->second_var();
> >+  const bool o_anon = !!is_anonymous_data_member(o);
> >+  const bool n_anon = !!is_anonymous_data_member(n);
> >+  const bool is_strict_anonymous_data_member_change = o_anon && n_anon;
> >+  const string o_name = o->get_qualified_name();
> >+  const string n_name = n->get_qualified_name();
> >+  const uint64_t o_size = get_var_size_in_bits(o);
> >+  const uint64_t n_size = get_var_size_in_bits(n);
> >+  const uint64_t o_offset = get_data_member_offset(o);
> >+  const uint64_t n_offset = get_data_member_offset(n);
> >+  const string o_pretty_representation = o->get_pretty_representation();
>
> Any reason to not have n_pretty_representation also already defined
> here?

That's what I had originally. However, it may be a fairly expensive
operation and it's only needed in relatively rare cases.

Regards,
Giuliano.

> Otherwise nice cleanup!
>
> Reviewed-by: Matthias Maennich <maennich@google.com>
>
> Cheers,
> Matthias
>
> >+
> >+  const bool show_size_offset_changes = ctxt->get_allowed_category()
> >+                                      & SIZE_OR_OFFSET_CHANGE_CATEGORY;
> >
> >   // Has the main diff text been output?
> >   bool emitted = false;
> >@@ -424,39 +431,31 @@ represent(const var_diff_sptr    &diff,
> >   bool begin_with_and = false;
> >   // Have we reported a size change already?
> >   bool size_reported = false;
> >-  // Are we reporting a change to an anonymous member?
> >-  bool is_strict_anonymous_data_member_change = false;
> >
> >-  string pretty_representation = o->get_pretty_representation();
> >-  bool show_size_offset_changes = ctxt->get_allowed_category()
> >-                                & SIZE_OR_OFFSET_CHANGE_CATEGORY;
> >-
> >-  if (is_anonymous_data_member(o) && is_anonymous_data_member(n))
> >+  if (is_strict_anonymous_data_member_change)
> >     {
> >-      is_strict_anonymous_data_member_change = true;
> >-      string tr1 = o->get_pretty_representation();
> >-      string tr2 = n->get_pretty_representation();
> >-      type_base_sptr t1 = o->get_type(), t2 = n->get_type();
> >-      if (tr1 != tr2)
> >+      const string n_pretty_representation = n->get_pretty_representation();
> >+      const type_base_sptr o_type = o->get_type(), n_type = n->get_type();
> >+      if (o_pretty_representation != n_pretty_representation)
> >       {
> >         show_offset_or_size(indent + "anonymous data member at offset",
> >-                            offset1, *ctxt, out);
> >+                            o_offset, *ctxt, out);
> >
> >         out << " changed from:\n"
> >-            << indent << "  " << tr1 << "\n"
> >+            << indent << "  " << o_pretty_representation << "\n"
> >             << indent << "to:\n"
> >-            << indent << "  " << tr2 << "\n";
> >+            << indent << "  " << n_pretty_representation << "\n";
> >
> >         begin_with_and = true;
> >         emitted = true;
> >       }
> >-      else if (get_type_name(t1) != get_type_name(t2)
> >-             && is_decl(t1) && is_decl(t2)
> >-             && is_decl(t1)->get_is_anonymous()
> >-             && is_decl(t2)->get_is_anonymous())
> >+      else if (get_type_name(o_type) != get_type_name(n_type)
> >+             && is_decl(o_type) && is_decl(n_type)
> >+             && is_decl(o_type)->get_is_anonymous()
> >+             && is_decl(n_type)->get_is_anonymous())
> >       {
> >         out << indent << "while looking at anonymous data member '"
> >-            << tr1 << "':\n"
> >+            << o_pretty_representation << "':\n"
> >             << indent << "the internal name of that anonymous data member"
> >                          "changed from:\n"
> >             << indent << " " << get_type_name(o->get_type()) << "\n"
> >@@ -472,36 +471,20 @@ represent(const var_diff_sptr    &diff,
> >     }
> >   else if (filtering::has_anonymous_data_member_change(diff))
> >     {
> >+      ABG_ASSERT(o_anon != n_anon);
> >       // So we are looking at a non-anonymous data member change from
> >       // or to anonymous data member.
> >-      if (is_anonymous_data_member(o))
> >-      {
> >-        string repr = "anonymous data member "
> >-          + o->get_pretty_representation()
> >-          + " at offset";
> >-
> >-        show_offset_or_size(indent + repr, offset1, *ctxt, out);
> >-        out << " became data member '"
> >-            << n->get_pretty_representation()
> >-            << "'\n";
> >-      }
> >-      else
> >-      {
> >-        ABG_ASSERT(is_anonymous_data_member(n));
> >-        string repr = "data member "
> >-          + o->get_pretty_representation()
> >-          + " at offset";
> >-
> >-        show_offset_or_size(indent + repr, offset1, *ctxt, out);
> >-        out << " became anonymous data member '"
> >-            << n->get_pretty_representation()
> >-            << "'\n";
> >-      }
> >+      const string n_pretty_representation = n->get_pretty_representation();
> >+      out << indent << (o_anon ? "anonymous " : "")
> >+        << "data member " << o_pretty_representation;
> >+      show_offset_or_size(" at offset", o_offset, *ctxt, out);
> >+      out << " became " << (n_anon ? "anonymous " : "")
> >+        << "data member '" << n_pretty_representation << "'\n";
> >
> >       begin_with_and = true;
> >       emitted = true;
> >     }
> >-  else if (diff_sptr d = diff->type_diff())
> >+  else if (const diff_sptr d = diff->type_diff())
> >     {
> >       if (ctxt->get_reporter()->diff_to_be_reported(d.get()))
> >       {
> >@@ -511,7 +494,7 @@ represent(const var_diff_sptr      &diff,
> >               << "' changed";
> >         else
> >           out << indent
> >-              << "type of '" << pretty_representation << "' changed";
> >+              << "type of '" << o_pretty_representation << "' changed";
> >
> >         if (d->currently_reporting())
> >           out << ", as being reported\n";
> >@@ -529,7 +512,7 @@ represent(const var_diff_sptr      &diff,
> >       }
> >     }
> >
> >-  if (!filtering::has_anonymous_data_member_change(diff) && name1 != name2)
> >+  if (!filtering::has_anonymous_data_member_change(diff) && o_name != n_name)
> >     {
> >       if (filtering::has_harmless_name_change(o, n)
> >         && !(ctxt->get_allowed_category()
> >@@ -546,7 +529,7 @@ represent(const var_diff_sptr      &diff,
> >           out << indent;
> >         else
> >           out << ", ";
> >-        out << "name of '" << name1 << "' changed to '" << name2 << "'";
> >+        out << "name of '" << o_name << "' changed to '" << n_name << "'";
> >         report_loc_info(n, *ctxt, out);
> >         emitted = true;
> >       }
> >@@ -561,7 +544,7 @@ represent(const var_diff_sptr      &diff,
> >         begin_with_and = false;
> >       }
> >       else if (!emitted)
> >-      out << indent << "'" << pretty_representation << "' ";
> >+      out << indent << "'" << o_pretty_representation << "' ";
> >       else
> >       out << ", ";
> >       if (get_data_member_is_laid_out(o))
> >@@ -572,7 +555,7 @@ represent(const var_diff_sptr      &diff,
> >     }
> >   if (show_size_offset_changes)
> >     {
> >-      if (offset1 != offset2)
> >+      if (o_offset != n_offset)
> >       {
> >         if (begin_with_and)
> >           {
> >@@ -584,17 +567,17 @@ represent(const var_diff_sptr    &diff,
> >             out << indent;
> >             if (is_strict_anonymous_data_member_change)
> >               out << "anonymous data member ";
> >-            out << "'" << pretty_representation << "' ";
> >+            out << "'" << o_pretty_representation << "' ";
> >           }
> >         else
> >           out << ", ";
> >
> >-        show_numerical_change("offset", offset1, offset2, *ctxt, out);
> >+        show_numerical_change("offset", o_offset, n_offset, *ctxt, out);
> >         maybe_show_relative_offset_change(diff, *ctxt, out);
> >         emitted = true;
> >       }
> >
> >-      if (!size_reported && size1 != size2)
> >+      if (!size_reported && o_size != n_size)
> >       {
> >         if (begin_with_and)
> >           {
> >@@ -606,12 +589,12 @@ represent(const var_diff_sptr    &diff,
> >             out << indent;
> >             if (is_strict_anonymous_data_member_change)
> >               out << "anonymous data member ";
> >-            out << "'" << pretty_representation << "' ";
> >+            out << "'" << o_pretty_representation << "' ";
> >           }
> >         else
> >           out << ", ";
> >
> >-        show_numerical_change("size", size1, size2, *ctxt, out);
> >+        show_numerical_change("size", o_size, n_size, *ctxt, out);
> >         maybe_show_relative_size_change(diff, *ctxt, out);
> >         emitted = true;
> >       }
> >@@ -624,7 +607,7 @@ represent(const var_diff_sptr      &diff,
> >         begin_with_and = false;
> >       }
> >       else if (!emitted)
> >-      out << indent << "'" << pretty_representation << "' ";
> >+      out << indent << "'" << o_pretty_representation << "' ";
> >       else
> >       out << ", ";
> >       out << "elf binding changed from " << o->get_binding()
> >@@ -639,7 +622,7 @@ represent(const var_diff_sptr      &diff,
> >         begin_with_and = false;
> >       }
> >       else if (!emitted)
> >-      out << indent << "'" << pretty_representation << "' ";
> >+      out << indent << "'" << o_pretty_representation << "' ";
> >       else
> >       out << ", ";
> >       out << "visibility changed from " << o->get_visibility()
> >@@ -656,7 +639,7 @@ represent(const var_diff_sptr      &diff,
> >         begin_with_and = false;
> >       }
> >       else if (!emitted)
> >-      out << indent << "'" << pretty_representation << "' ";
> >+      out << indent << "'" << o_pretty_representation << "' ";
> >       else
> >       out << ", ";
> >
> >@@ -675,7 +658,7 @@ represent(const var_diff_sptr      &diff,
> >         begin_with_and = false;
> >       }
> >       else if (!emitted)
> >-      out << indent << "'" << pretty_representation << "' ";
> >+      out << indent << "'" << o_pretty_representation << "' ";
> >       else
> >       out << ", ";
> >
> >@@ -691,7 +674,7 @@ represent(const var_diff_sptr      &diff,
> >     ;
> >   else if (!emitted)
> >     // We appear to have fallen off the edge of the map.
> >-    out << indent << "'" << pretty_representation
> >+    out << indent << "'" << o_pretty_representation
> >       << "' has *some* difference - please report as a bug";
> >   else
> >     // do nothing
> >--
> >2.26.0.110.g2183baf09c-goog
> >
  

On Wed, Apr 08, 2020 at 03:24:24PM +0100, Giuliano Procida wrote:
>Hi.
>
>On Wed, 8 Apr 2020 at 13:47, Matthias Maennich <maennich@google.com> wrote:
>>
>> On Wed, Apr 08, 2020 at 01:20:43PM +0100, Giuliano Procida wrote:
>> >This change:
>> >
>> >    - makes local variables const where possible
>> >    - gives local variables more descriptive names
>> >    - factors out some more expressions as local variables
>> >    - simplifies the logic around anonymous data members
>> >
>> >There are no behavioural changes.
>> >
>> >       * src/abg-reporter-priv.cc (represent): In the var_diff_sptr
>> >       overload, rename pretty_representation to
>> >       o_pretty_representation, introduce n_pretty_representation
>> >       where needed and replace the duplicate tr1 and tr2 with these;
>> >       rename all other variables foo1 and foo2 to o_foo and n_foo
>> >       respectively, using _type instead of _t; introduce o_anon and
>> >       n_anon and use them to make the local variable
>> >       is_strict_anonymous_data_member_change const, make ABG_ASSERT
>> >       in anonymous data member handling more obvious in the case
>> >       where anonymity has changed and allow simplification of
>> >       formatting in this case; move declarations of const local
>> >       variables above those of the non-const, state-tracking,
>> >       variables.
>> >
>> >Signed-off-by: Giuliano Procida <gprocida@google.com>
>> >---
>> > src/abg-reporter-priv.cc | 117 +++++++++++++++++----------------------
>> > 1 file changed, 50 insertions(+), 67 deletions(-)
>> >
>> >diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc
>> >index 5fb5fbc5..a46c0e9d 100644
>> >--- a/src/abg-reporter-priv.cc
>> >+++ b/src/abg-reporter-priv.cc
>> >@@ -409,14 +409,21 @@ represent(const var_diff_sptr    &diff,
>> >   if (!ctxt->get_reporter()->diff_to_be_reported(diff.get()))
>> >     return;
>> >
>> >-  var_decl_sptr o = diff->first_var();
>> >-  var_decl_sptr n = diff->second_var();
>> >-  string name1 = o->get_qualified_name();
>> >-  string name2 = n->get_qualified_name();
>> >-  uint64_t size1 = get_var_size_in_bits(o);
>> >-  uint64_t size2 = get_var_size_in_bits(n);
>> >-  uint64_t offset1 = get_data_member_offset(o);
>> >-  uint64_t offset2 = get_data_member_offset(n);
>> >+  const var_decl_sptr o = diff->first_var();
>> >+  const var_decl_sptr n = diff->second_var();
>> >+  const bool o_anon = !!is_anonymous_data_member(o);
>> >+  const bool n_anon = !!is_anonymous_data_member(n);
>> >+  const bool is_strict_anonymous_data_member_change = o_anon && n_anon;
>> >+  const string o_name = o->get_qualified_name();
>> >+  const string n_name = n->get_qualified_name();
>> >+  const uint64_t o_size = get_var_size_in_bits(o);
>> >+  const uint64_t n_size = get_var_size_in_bits(n);
>> >+  const uint64_t o_offset = get_data_member_offset(o);
>> >+  const uint64_t n_offset = get_data_member_offset(n);
>> >+  const string o_pretty_representation = o->get_pretty_representation();
>>
>> Any reason to not have n_pretty_representation also already defined
>> here?
>
>That's what I had originally. However, it may be a fairly expensive
>operation and it's only needed in relatively rare cases.

Fair enough. Maybe add a comment then.

Cheers,
Matthias

>
>Regards,
>Giuliano.
>
>> Otherwise nice cleanup!
>>
>> Reviewed-by: Matthias Maennich <maennich@google.com>
>>
>> Cheers,
>> Matthias
>>
>> >+
>> >+  const bool show_size_offset_changes = ctxt->get_allowed_category()
>> >+                                      & SIZE_OR_OFFSET_CHANGE_CATEGORY;
>> >
>> >   // Has the main diff text been output?
>> >   bool emitted = false;
>> >@@ -424,39 +431,31 @@ represent(const var_diff_sptr    &diff,
>> >   bool begin_with_and = false;
>> >   // Have we reported a size change already?
>> >   bool size_reported = false;
>> >-  // Are we reporting a change to an anonymous member?
>> >-  bool is_strict_anonymous_data_member_change = false;
>> >
>> >-  string pretty_representation = o->get_pretty_representation();
>> >-  bool show_size_offset_changes = ctxt->get_allowed_category()
>> >-                                & SIZE_OR_OFFSET_CHANGE_CATEGORY;
>> >-
>> >-  if (is_anonymous_data_member(o) && is_anonymous_data_member(n))
>> >+  if (is_strict_anonymous_data_member_change)
>> >     {
>> >-      is_strict_anonymous_data_member_change = true;
>> >-      string tr1 = o->get_pretty_representation();
>> >-      string tr2 = n->get_pretty_representation();
>> >-      type_base_sptr t1 = o->get_type(), t2 = n->get_type();
>> >-      if (tr1 != tr2)
>> >+      const string n_pretty_representation = n->get_pretty_representation();
>> >+      const type_base_sptr o_type = o->get_type(), n_type = n->get_type();
>> >+      if (o_pretty_representation != n_pretty_representation)
>> >       {
>> >         show_offset_or_size(indent + "anonymous data member at offset",
>> >-                            offset1, *ctxt, out);
>> >+                            o_offset, *ctxt, out);
>> >
>> >         out << " changed from:\n"
>> >-            << indent << "  " << tr1 << "\n"
>> >+            << indent << "  " << o_pretty_representation << "\n"
>> >             << indent << "to:\n"
>> >-            << indent << "  " << tr2 << "\n";
>> >+            << indent << "  " << n_pretty_representation << "\n";
>> >
>> >         begin_with_and = true;
>> >         emitted = true;
>> >       }
>> >-      else if (get_type_name(t1) != get_type_name(t2)
>> >-             && is_decl(t1) && is_decl(t2)
>> >-             && is_decl(t1)->get_is_anonymous()
>> >-             && is_decl(t2)->get_is_anonymous())
>> >+      else if (get_type_name(o_type) != get_type_name(n_type)
>> >+             && is_decl(o_type) && is_decl(n_type)
>> >+             && is_decl(o_type)->get_is_anonymous()
>> >+             && is_decl(n_type)->get_is_anonymous())
>> >       {
>> >         out << indent << "while looking at anonymous data member '"
>> >-            << tr1 << "':\n"
>> >+            << o_pretty_representation << "':\n"
>> >             << indent << "the internal name of that anonymous data member"
>> >                          "changed from:\n"
>> >             << indent << " " << get_type_name(o->get_type()) << "\n"
>> >@@ -472,36 +471,20 @@ represent(const var_diff_sptr    &diff,
>> >     }
>> >   else if (filtering::has_anonymous_data_member_change(diff))
>> >     {
>> >+      ABG_ASSERT(o_anon != n_anon);
>> >       // So we are looking at a non-anonymous data member change from
>> >       // or to anonymous data member.
>> >-      if (is_anonymous_data_member(o))
>> >-      {
>> >-        string repr = "anonymous data member "
>> >-          + o->get_pretty_representation()
>> >-          + " at offset";
>> >-
>> >-        show_offset_or_size(indent + repr, offset1, *ctxt, out);
>> >-        out << " became data member '"
>> >-            << n->get_pretty_representation()
>> >-            << "'\n";
>> >-      }
>> >-      else
>> >-      {
>> >-        ABG_ASSERT(is_anonymous_data_member(n));
>> >-        string repr = "data member "
>> >-          + o->get_pretty_representation()
>> >-          + " at offset";
>> >-
>> >-        show_offset_or_size(indent + repr, offset1, *ctxt, out);
>> >-        out << " became anonymous data member '"
>> >-            << n->get_pretty_representation()
>> >-            << "'\n";
>> >-      }
>> >+      const string n_pretty_representation = n->get_pretty_representation();
>> >+      out << indent << (o_anon ? "anonymous " : "")
>> >+        << "data member " << o_pretty_representation;
>> >+      show_offset_or_size(" at offset", o_offset, *ctxt, out);
>> >+      out << " became " << (n_anon ? "anonymous " : "")
>> >+        << "data member '" << n_pretty_representation << "'\n";
>> >
>> >       begin_with_and = true;
>> >       emitted = true;
>> >     }
>> >-  else if (diff_sptr d = diff->type_diff())
>> >+  else if (const diff_sptr d = diff->type_diff())
>> >     {
>> >       if (ctxt->get_reporter()->diff_to_be_reported(d.get()))
>> >       {
>> >@@ -511,7 +494,7 @@ represent(const var_diff_sptr      &diff,
>> >               << "' changed";
>> >         else
>> >           out << indent
>> >-              << "type of '" << pretty_representation << "' changed";
>> >+              << "type of '" << o_pretty_representation << "' changed";
>> >
>> >         if (d->currently_reporting())
>> >           out << ", as being reported\n";
>> >@@ -529,7 +512,7 @@ represent(const var_diff_sptr      &diff,
>> >       }
>> >     }
>> >
>> >-  if (!filtering::has_anonymous_data_member_change(diff) && name1 != name2)
>> >+  if (!filtering::has_anonymous_data_member_change(diff) && o_name != n_name)
>> >     {
>> >       if (filtering::has_harmless_name_change(o, n)
>> >         && !(ctxt->get_allowed_category()
>> >@@ -546,7 +529,7 @@ represent(const var_diff_sptr      &diff,
>> >           out << indent;
>> >         else
>> >           out << ", ";
>> >-        out << "name of '" << name1 << "' changed to '" << name2 << "'";
>> >+        out << "name of '" << o_name << "' changed to '" << n_name << "'";
>> >         report_loc_info(n, *ctxt, out);
>> >         emitted = true;
>> >       }
>> >@@ -561,7 +544,7 @@ represent(const var_diff_sptr      &diff,
>> >         begin_with_and = false;
>> >       }
>> >       else if (!emitted)
>> >-      out << indent << "'" << pretty_representation << "' ";
>> >+      out << indent << "'" << o_pretty_representation << "' ";
>> >       else
>> >       out << ", ";
>> >       if (get_data_member_is_laid_out(o))
>> >@@ -572,7 +555,7 @@ represent(const var_diff_sptr      &diff,
>> >     }
>> >   if (show_size_offset_changes)
>> >     {
>> >-      if (offset1 != offset2)
>> >+      if (o_offset != n_offset)
>> >       {
>> >         if (begin_with_and)
>> >           {
>> >@@ -584,17 +567,17 @@ represent(const var_diff_sptr    &diff,
>> >             out << indent;
>> >             if (is_strict_anonymous_data_member_change)
>> >               out << "anonymous data member ";
>> >-            out << "'" << pretty_representation << "' ";
>> >+            out << "'" << o_pretty_representation << "' ";
>> >           }
>> >         else
>> >           out << ", ";
>> >
>> >-        show_numerical_change("offset", offset1, offset2, *ctxt, out);
>> >+        show_numerical_change("offset", o_offset, n_offset, *ctxt, out);
>> >         maybe_show_relative_offset_change(diff, *ctxt, out);
>> >         emitted = true;
>> >       }
>> >
>> >-      if (!size_reported && size1 != size2)
>> >+      if (!size_reported && o_size != n_size)
>> >       {
>> >         if (begin_with_and)
>> >           {
>> >@@ -606,12 +589,12 @@ represent(const var_diff_sptr    &diff,
>> >             out << indent;
>> >             if (is_strict_anonymous_data_member_change)
>> >               out << "anonymous data member ";
>> >-            out << "'" << pretty_representation << "' ";
>> >+            out << "'" << o_pretty_representation << "' ";
>> >           }
>> >         else
>> >           out << ", ";
>> >
>> >-        show_numerical_change("size", size1, size2, *ctxt, out);
>> >+        show_numerical_change("size", o_size, n_size, *ctxt, out);
>> >         maybe_show_relative_size_change(diff, *ctxt, out);
>> >         emitted = true;
>> >       }
>> >@@ -624,7 +607,7 @@ represent(const var_diff_sptr      &diff,
>> >         begin_with_and = false;
>> >       }
>> >       else if (!emitted)
>> >-      out << indent << "'" << pretty_representation << "' ";
>> >+      out << indent << "'" << o_pretty_representation << "' ";
>> >       else
>> >       out << ", ";
>> >       out << "elf binding changed from " << o->get_binding()
>> >@@ -639,7 +622,7 @@ represent(const var_diff_sptr      &diff,
>> >         begin_with_and = false;
>> >       }
>> >       else if (!emitted)
>> >-      out << indent << "'" << pretty_representation << "' ";
>> >+      out << indent << "'" << o_pretty_representation << "' ";
>> >       else
>> >       out << ", ";
>> >       out << "visibility changed from " << o->get_visibility()
>> >@@ -656,7 +639,7 @@ represent(const var_diff_sptr      &diff,
>> >         begin_with_and = false;
>> >       }
>> >       else if (!emitted)
>> >-      out << indent << "'" << pretty_representation << "' ";
>> >+      out << indent << "'" << o_pretty_representation << "' ";
>> >       else
>> >       out << ", ";
>> >
>> >@@ -675,7 +658,7 @@ represent(const var_diff_sptr      &diff,
>> >         begin_with_and = false;
>> >       }
>> >       else if (!emitted)
>> >-      out << indent << "'" << pretty_representation << "' ";
>> >+      out << indent << "'" << o_pretty_representation << "' ";
>> >       else
>> >       out << ", ";
>> >
>> >@@ -691,7 +674,7 @@ represent(const var_diff_sptr      &diff,
>> >     ;
>> >   else if (!emitted)
>> >     // We appear to have fallen off the edge of the map.
>> >-    out << indent << "'" << pretty_representation
>> >+    out << indent << "'" << o_pretty_representation
>> >       << "' has *some* difference - please report as a bug";
>> >   else
>> >     // do nothing
>> >--
>> >2.26.0.110.g2183baf09c-goog
>> >
  

On Wed, 8 Apr 2020 at 16:29, Matthias Maennich <maennich@google.com> wrote:
>
> On Wed, Apr 08, 2020 at 03:24:24PM +0100, Giuliano Procida wrote:
> >Hi.
> >
> >On Wed, 8 Apr 2020 at 13:47, Matthias Maennich <maennich@google.com> wrote:
> >>
> >> On Wed, Apr 08, 2020 at 01:20:43PM +0100, Giuliano Procida wrote:
> >> >This change:
> >> >
> >> >    - makes local variables const where possible
> >> >    - gives local variables more descriptive names
> >> >    - factors out some more expressions as local variables
> >> >    - simplifies the logic around anonymous data members
> >> >
> >> >There are no behavioural changes.
> >> >
> >> >       * src/abg-reporter-priv.cc (represent): In the var_diff_sptr
> >> >       overload, rename pretty_representation to
> >> >       o_pretty_representation, introduce n_pretty_representation
> >> >       where needed and replace the duplicate tr1 and tr2 with these;
> >> >       rename all other variables foo1 and foo2 to o_foo and n_foo
> >> >       respectively, using _type instead of _t; introduce o_anon and
> >> >       n_anon and use them to make the local variable
> >> >       is_strict_anonymous_data_member_change const, make ABG_ASSERT
> >> >       in anonymous data member handling more obvious in the case
> >> >       where anonymity has changed and allow simplification of
> >> >       formatting in this case; move declarations of const local
> >> >       variables above those of the non-const, state-tracking,
> >> >       variables.
> >> >
> >> >Signed-off-by: Giuliano Procida <gprocida@google.com>
> >> >---
> >> > src/abg-reporter-priv.cc | 117 +++++++++++++++++----------------------
> >> > 1 file changed, 50 insertions(+), 67 deletions(-)
> >> >
> >> >diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc
> >> >index 5fb5fbc5..a46c0e9d 100644
> >> >--- a/src/abg-reporter-priv.cc
> >> >+++ b/src/abg-reporter-priv.cc
> >> >@@ -409,14 +409,21 @@ represent(const var_diff_sptr    &diff,
> >> >   if (!ctxt->get_reporter()->diff_to_be_reported(diff.get()))
> >> >     return;
> >> >
> >> >-  var_decl_sptr o = diff->first_var();
> >> >-  var_decl_sptr n = diff->second_var();
> >> >-  string name1 = o->get_qualified_name();
> >> >-  string name2 = n->get_qualified_name();
> >> >-  uint64_t size1 = get_var_size_in_bits(o);
> >> >-  uint64_t size2 = get_var_size_in_bits(n);
> >> >-  uint64_t offset1 = get_data_member_offset(o);
> >> >-  uint64_t offset2 = get_data_member_offset(n);
> >> >+  const var_decl_sptr o = diff->first_var();
> >> >+  const var_decl_sptr n = diff->second_var();
> >> >+  const bool o_anon = !!is_anonymous_data_member(o);
> >> >+  const bool n_anon = !!is_anonymous_data_member(n);
> >> >+  const bool is_strict_anonymous_data_member_change = o_anon && n_anon;
> >> >+  const string o_name = o->get_qualified_name();
> >> >+  const string n_name = n->get_qualified_name();
> >> >+  const uint64_t o_size = get_var_size_in_bits(o);
> >> >+  const uint64_t n_size = get_var_size_in_bits(n);
> >> >+  const uint64_t o_offset = get_data_member_offset(o);
> >> >+  const uint64_t n_offset = get_data_member_offset(n);
> >> >+  const string o_pretty_representation = o->get_pretty_representation();
> >>
> >> Any reason to not have n_pretty_representation also already defined
> >> here?
> >
> >That's what I had originally. However, it may be a fairly expensive
> >operation and it's only needed in relatively rare cases.
>
> Fair enough. Maybe add a comment then.

Will do. Also spotted a missed subexpression elimination. Revision
patch on the way.

Giuliano.

> Cheers,
> Matthias
>
> >
> >Regards,
> >Giuliano.
> >
> >> Otherwise nice cleanup!
> >>
> >> Reviewed-by: Matthias Maennich <maennich@google.com>
> >>
> >> Cheers,
> >> Matthias
> >>
> >> >+
> >> >+  const bool show_size_offset_changes = ctxt->get_allowed_category()
> >> >+                                      & SIZE_OR_OFFSET_CHANGE_CATEGORY;
> >> >
> >> >   // Has the main diff text been output?
> >> >   bool emitted = false;
> >> >@@ -424,39 +431,31 @@ represent(const var_diff_sptr    &diff,
> >> >   bool begin_with_and = false;
> >> >   // Have we reported a size change already?
> >> >   bool size_reported = false;
> >> >-  // Are we reporting a change to an anonymous member?
> >> >-  bool is_strict_anonymous_data_member_change = false;
> >> >
> >> >-  string pretty_representation = o->get_pretty_representation();
> >> >-  bool show_size_offset_changes = ctxt->get_allowed_category()
> >> >-                                & SIZE_OR_OFFSET_CHANGE_CATEGORY;
> >> >-
> >> >-  if (is_anonymous_data_member(o) && is_anonymous_data_member(n))
> >> >+  if (is_strict_anonymous_data_member_change)
> >> >     {
> >> >-      is_strict_anonymous_data_member_change = true;
> >> >-      string tr1 = o->get_pretty_representation();
> >> >-      string tr2 = n->get_pretty_representation();
> >> >-      type_base_sptr t1 = o->get_type(), t2 = n->get_type();
> >> >-      if (tr1 != tr2)
> >> >+      const string n_pretty_representation = n->get_pretty_representation();
> >> >+      const type_base_sptr o_type = o->get_type(), n_type = n->get_type();
> >> >+      if (o_pretty_representation != n_pretty_representation)
> >> >       {
> >> >         show_offset_or_size(indent + "anonymous data member at offset",
> >> >-                            offset1, *ctxt, out);
> >> >+                            o_offset, *ctxt, out);
> >> >
> >> >         out << " changed from:\n"
> >> >-            << indent << "  " << tr1 << "\n"
> >> >+            << indent << "  " << o_pretty_representation << "\n"
> >> >             << indent << "to:\n"
> >> >-            << indent << "  " << tr2 << "\n";
> >> >+            << indent << "  " << n_pretty_representation << "\n";
> >> >
> >> >         begin_with_and = true;
> >> >         emitted = true;
> >> >       }
> >> >-      else if (get_type_name(t1) != get_type_name(t2)
> >> >-             && is_decl(t1) && is_decl(t2)
> >> >-             && is_decl(t1)->get_is_anonymous()
> >> >-             && is_decl(t2)->get_is_anonymous())
> >> >+      else if (get_type_name(o_type) != get_type_name(n_type)
> >> >+             && is_decl(o_type) && is_decl(n_type)
> >> >+             && is_decl(o_type)->get_is_anonymous()
> >> >+             && is_decl(n_type)->get_is_anonymous())
> >> >       {
> >> >         out << indent << "while looking at anonymous data member '"
> >> >-            << tr1 << "':\n"
> >> >+            << o_pretty_representation << "':\n"
> >> >             << indent << "the internal name of that anonymous data member"
> >> >                          "changed from:\n"
> >> >             << indent << " " << get_type_name(o->get_type()) << "\n"
> >> >@@ -472,36 +471,20 @@ represent(const var_diff_sptr    &diff,
> >> >     }
> >> >   else if (filtering::has_anonymous_data_member_change(diff))
> >> >     {
> >> >+      ABG_ASSERT(o_anon != n_anon);
> >> >       // So we are looking at a non-anonymous data member change from
> >> >       // or to anonymous data member.
> >> >-      if (is_anonymous_data_member(o))
> >> >-      {
> >> >-        string repr = "anonymous data member "
> >> >-          + o->get_pretty_representation()
> >> >-          + " at offset";
> >> >-
> >> >-        show_offset_or_size(indent + repr, offset1, *ctxt, out);
> >> >-        out << " became data member '"
> >> >-            << n->get_pretty_representation()
> >> >-            << "'\n";
> >> >-      }
> >> >-      else
> >> >-      {
> >> >-        ABG_ASSERT(is_anonymous_data_member(n));
> >> >-        string repr = "data member "
> >> >-          + o->get_pretty_representation()
> >> >-          + " at offset";
> >> >-
> >> >-        show_offset_or_size(indent + repr, offset1, *ctxt, out);
> >> >-        out << " became anonymous data member '"
> >> >-            << n->get_pretty_representation()
> >> >-            << "'\n";
> >> >-      }
> >> >+      const string n_pretty_representation = n->get_pretty_representation();
> >> >+      out << indent << (o_anon ? "anonymous " : "")
> >> >+        << "data member " << o_pretty_representation;
> >> >+      show_offset_or_size(" at offset", o_offset, *ctxt, out);
> >> >+      out << " became " << (n_anon ? "anonymous " : "")
> >> >+        << "data member '" << n_pretty_representation << "'\n";
> >> >
> >> >       begin_with_and = true;
> >> >       emitted = true;
> >> >     }
> >> >-  else if (diff_sptr d = diff->type_diff())
> >> >+  else if (const diff_sptr d = diff->type_diff())
> >> >     {
> >> >       if (ctxt->get_reporter()->diff_to_be_reported(d.get()))
> >> >       {
> >> >@@ -511,7 +494,7 @@ represent(const var_diff_sptr      &diff,
> >> >               << "' changed";
> >> >         else
> >> >           out << indent
> >> >-              << "type of '" << pretty_representation << "' changed";
> >> >+              << "type of '" << o_pretty_representation << "' changed";
> >> >
> >> >         if (d->currently_reporting())
> >> >           out << ", as being reported\n";
> >> >@@ -529,7 +512,7 @@ represent(const var_diff_sptr      &diff,
> >> >       }
> >> >     }
> >> >
> >> >-  if (!filtering::has_anonymous_data_member_change(diff) && name1 != name2)
> >> >+  if (!filtering::has_anonymous_data_member_change(diff) && o_name != n_name)
> >> >     {
> >> >       if (filtering::has_harmless_name_change(o, n)
> >> >         && !(ctxt->get_allowed_category()
> >> >@@ -546,7 +529,7 @@ represent(const var_diff_sptr      &diff,
> >> >           out << indent;
> >> >         else
> >> >           out << ", ";
> >> >-        out << "name of '" << name1 << "' changed to '" << name2 << "'";
> >> >+        out << "name of '" << o_name << "' changed to '" << n_name << "'";
> >> >         report_loc_info(n, *ctxt, out);
> >> >         emitted = true;
> >> >       }
> >> >@@ -561,7 +544,7 @@ represent(const var_diff_sptr      &diff,
> >> >         begin_with_and = false;
> >> >       }
> >> >       else if (!emitted)
> >> >-      out << indent << "'" << pretty_representation << "' ";
> >> >+      out << indent << "'" << o_pretty_representation << "' ";
> >> >       else
> >> >       out << ", ";
> >> >       if (get_data_member_is_laid_out(o))
> >> >@@ -572,7 +555,7 @@ represent(const var_diff_sptr      &diff,
> >> >     }
> >> >   if (show_size_offset_changes)
> >> >     {
> >> >-      if (offset1 != offset2)
> >> >+      if (o_offset != n_offset)
> >> >       {
> >> >         if (begin_with_and)
> >> >           {
> >> >@@ -584,17 +567,17 @@ represent(const var_diff_sptr    &diff,
> >> >             out << indent;
> >> >             if (is_strict_anonymous_data_member_change)
> >> >               out << "anonymous data member ";
> >> >-            out << "'" << pretty_representation << "' ";
> >> >+            out << "'" << o_pretty_representation << "' ";
> >> >           }
> >> >         else
> >> >           out << ", ";
> >> >
> >> >-        show_numerical_change("offset", offset1, offset2, *ctxt, out);
> >> >+        show_numerical_change("offset", o_offset, n_offset, *ctxt, out);
> >> >         maybe_show_relative_offset_change(diff, *ctxt, out);
> >> >         emitted = true;
> >> >       }
> >> >
> >> >-      if (!size_reported && size1 != size2)
> >> >+      if (!size_reported && o_size != n_size)
> >> >       {
> >> >         if (begin_with_and)
> >> >           {
> >> >@@ -606,12 +589,12 @@ represent(const var_diff_sptr    &diff,
> >> >             out << indent;
> >> >             if (is_strict_anonymous_data_member_change)
> >> >               out << "anonymous data member ";
> >> >-            out << "'" << pretty_representation << "' ";
> >> >+            out << "'" << o_pretty_representation << "' ";
> >> >           }
> >> >         else
> >> >           out << ", ";
> >> >
> >> >-        show_numerical_change("size", size1, size2, *ctxt, out);
> >> >+        show_numerical_change("size", o_size, n_size, *ctxt, out);
> >> >         maybe_show_relative_size_change(diff, *ctxt, out);
> >> >         emitted = true;
> >> >       }
> >> >@@ -624,7 +607,7 @@ represent(const var_diff_sptr      &diff,
> >> >         begin_with_and = false;
> >> >       }
> >> >       else if (!emitted)
> >> >-      out << indent << "'" << pretty_representation << "' ";
> >> >+      out << indent << "'" << o_pretty_representation << "' ";
> >> >       else
> >> >       out << ", ";
> >> >       out << "elf binding changed from " << o->get_binding()
> >> >@@ -639,7 +622,7 @@ represent(const var_diff_sptr      &diff,
> >> >         begin_with_and = false;
> >> >       }
> >> >       else if (!emitted)
> >> >-      out << indent << "'" << pretty_representation << "' ";
> >> >+      out << indent << "'" << o_pretty_representation << "' ";
> >> >       else
> >> >       out << ", ";
> >> >       out << "visibility changed from " << o->get_visibility()
> >> >@@ -656,7 +639,7 @@ represent(const var_diff_sptr      &diff,
> >> >         begin_with_and = false;
> >> >       }
> >> >       else if (!emitted)
> >> >-      out << indent << "'" << pretty_representation << "' ";
> >> >+      out << indent << "'" << o_pretty_representation << "' ";
> >> >       else
> >> >       out << ", ";
> >> >
> >> >@@ -675,7 +658,7 @@ represent(const var_diff_sptr      &diff,
> >> >         begin_with_and = false;
> >> >       }
> >> >       else if (!emitted)
> >> >-      out << indent << "'" << pretty_representation << "' ";
> >> >+      out << indent << "'" << o_pretty_representation << "' ";
> >> >       else
> >> >       out << ", ";
> >> >
> >> >@@ -691,7 +674,7 @@ represent(const var_diff_sptr      &diff,
> >> >     ;
> >> >   else if (!emitted)
> >> >     // We appear to have fallen off the edge of the map.
> >> >-    out << indent << "'" << pretty_representation
> >> >+    out << indent << "'" << o_pretty_representation
> >> >       << "' has *some* difference - please report as a bug";
> >> >   else
> >> >     // do nothing
> >> >--
> >> >2.26.0.110.g2183baf09c-goog
> >> >