From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <3pcGNXggKClo8HJG4A528GG8D6.4GEDA323A82ADKGMJ46O2J6.GJ8@flex--gprocida.bounces.google.com> Received: from mail-wr1-x44a.google.com (mail-wr1-x44a.google.com [IPv6:2a00:1450:4864:20::44a]) by sourceware.org (Postfix) with ESMTPS id 6263D385BF81 for ; Wed, 8 Apr 2020 12:20:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 6263D385BF81 Received: by mail-wr1-x44a.google.com with SMTP id h95so3861002wrh.11 for ; Wed, 08 Apr 2020 05:20:55 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:message-id:mime-version:subject:from:to:cc; bh=LsaB7u/2XHd7rLJBp8nessjZbRcq6gxNd0PjGPFLpq4=; b=ejaBsw81R/xtO84twdCtNCcL2XrjV2klI8Z8VbRhzbjRvZdCV2kTFTfv/twjYQFAj9 U3orz5rC9sqJhOLaU0aA1u3+yr6slsgSMPXtNoYIKMiW37RAuySH1+cLY1OjbTFHIAKY 6V8iTidq+RYAbiDdy2h5duxURvdu9e/qiH1SgKAAC/yOwO3u95q3q1/C8ZbmUPYinOhm 37caOtNpcjXeI2CocE3dZdFfsSwOYZjVU06EfrCLzwn7/P8aGYjF4zULi7SIAse48wqx kPmvbo1PM3+ixcmtv1FTzmNcLTGFSH4tcWpLDGWJfU3He1fhrmlqumSd3a+OfQo3wlfr ADeg== X-Gm-Message-State: AGi0PuaRz21XEGkNBIIqxRVDpdU634+0Cj4VTmqHNAqbvl/WiV87BofU C27xix/yF6aIa416H/5lT5NP43n7gIIstK9QtPMVwgWE0Kfa2lY3VLcbiztmGmWjlNzejCPy1vt PwiyNhCaHtoyUx+I5ICKpPUNsKY0PEFvh2VokRklPYnS1ZnvstG3ioE84yRbpcrUnuj7rvQA= X-Google-Smtp-Source: APiQypJQPLXR+x0Vy85ClGw9Ci1E5tTyWrIggpVrkZsez77tKkImkMvYJDRngLvGouEvz9uMwUWb5cdbesP5fA== X-Received: by 2002:adf:e90b:: with SMTP id f11mr8047911wrm.65.1586348453962; Wed, 08 Apr 2020 05:20:53 -0700 (PDT) Date: Wed, 8 Apr 2020 13:20:43 +0100 Message-Id: <20200408122044.192132-1-gprocida@google.com> Mime-Version: 1.0 X-Mailer: git-send-email 2.26.0.110.g2183baf09c-goog Subject: [PATCH 1/2] Improve readability of represent helper function. From: Giuliano Procida To: libabigail@sourceware.org Cc: dodji@seketeli.org, kernel-team@android.com, gprocida@google.com, maennich@google.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-34.0 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, 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 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 12:20:57 -0000 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(); + + 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