From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io1-xd42.google.com (mail-io1-xd42.google.com [IPv6:2607:f8b0:4864:20::d42]) by sourceware.org (Postfix) with ESMTPS id 9811C388702E for ; Wed, 8 Apr 2020 16:38:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 9811C388702E Received: by mail-io1-xd42.google.com with SMTP id f19so743694iog.5 for ; Wed, 08 Apr 2020 09:38:52 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=N9VsTIu8pwc1XlcKWWRNuEpoguMggZgfALyiOLnGKMg=; b=Hr1nKajj2qgOwNrfXKV4SYtr1r40EPou3vKM6ZeDVcCAzUNObiHFgCL2aCMxStSGM3 7MwevCQNUsKcd7vol9/K7kvwHH+O349LoDvFura0rWFcpg8rFmbEVBKtB26FrY4KuXkE qJsoiqGyo8XesAQzc3v59L+m1euTIWH+o2SpNZLJYQun5fkLlEgdiNWmjMUmuxO9ddeR tDeXq+9Xlr/6Phcd50AfRBep3klNUahbFZGqNJZy6UArG9n6ntKQHcnI4auwUoZJzpS/ Vi+wSO3moJmvAO8opMBj0hqzy8Hu2dBm7IrYBtMKq7RRn4/vxp0Sn03GgKRoMDqfgOVn 9zng== X-Gm-Message-State: AGi0Pubob1oI5qf7i6+gc2Ukcl4gtoZhn0WeDwQSTPNLIAO9bfMQOl07 p50HMFCEDmn7D2ubk9XGEqZo+7i8eiJ0rUSSKLsW4w== X-Google-Smtp-Source: APiQypKQWShhl8l0LDDLKcTCWofiInNTjZaKQoRVag5ZNSPIYomORsb1WnLSKMjvX9y5tElrWOl30J9CFA4MynIt18w= X-Received: by 2002:a05:6602:2cd3:: with SMTP id j19mr7914585iow.204.1586363931643; Wed, 08 Apr 2020 09:38:51 -0700 (PDT) MIME-Version: 1.0 References: <20200408122044.192132-1-gprocida@google.com> <20200408124731.GG189388@google.com> <20200408145717.GK189388@google.com> In-Reply-To: <20200408145717.GK189388@google.com> From: Giuliano Procida Date: Wed, 8 Apr 2020 17:38:34 +0100 Message-ID: Subject: Re: [PATCH 1/2] Improve readability of represent helper function. To: Matthias Maennich Cc: libabigail@sourceware.org, Dodji Seketeli , kernel-team@android.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-38.3 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libabigail mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Apr 2020 16:38:54 -0000 On Wed, 8 Apr 2020 at 16:29, Matthias Maennich 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 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 > >> >--- > >> > 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 > >> > >> 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 > >> >