From: Giuliano Procida <gprocida@google.com>
To: Matthias Maennich <maennich@google.com>
Cc: libabigail@sourceware.org, Dodji Seketeli <dodji@seketeli.org>,
kernel-team@android.com
Subject: Re: [PATCH 1/2] Improve readability of represent helper function.
Date: Wed, 8 Apr 2020 17:38:34 +0100 [thread overview]
Message-ID: <CAGvU0HkyQZtUWiN1NgNLjRhjVpR5uF1m-=OLGss0MSzpeTpL3A@mail.gmail.com> (raw)
In-Reply-To: <20200408145717.GK189388@google.com>
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
> >> >
next prev parent reply other threads:[~2020-04-08 16:38 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-08 12:20 Giuliano Procida
2020-04-08 12:20 ` [PATCH 2/2] abidiff: Document and refresh tests Giuliano Procida
2020-04-08 12:54 ` Matthias Maennich
2020-04-14 11:24 ` Dodji Seketeli
2020-04-08 12:47 ` [PATCH 1/2] Improve readability of represent helper function Matthias Maennich
2020-04-08 14:24 ` Giuliano Procida
2020-04-08 14:57 ` Matthias Maennich
2020-04-08 16:38 ` Giuliano Procida [this message]
2020-04-08 16:40 ` [PATCH v2 " Giuliano Procida
2020-04-14 11:09 ` Dodji Seketeli
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAGvU0HkyQZtUWiN1NgNLjRhjVpR5uF1m-=OLGss0MSzpeTpL3A@mail.gmail.com' \
--to=gprocida@google.com \
--cc=dodji@seketeli.org \
--cc=kernel-team@android.com \
--cc=libabigail@sourceware.org \
--cc=maennich@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).