From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net [217.70.183.195]) by sourceware.org (Postfix) with ESMTPS id 15E593858D35 for ; Mon, 27 Jul 2020 07:56:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 15E593858D35 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=seketeli.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=dodji@seketeli.org X-Originating-IP: 91.166.131.130 Received: from localhost (91-166-131-130.subs.proxad.net [91.166.131.130]) (Authenticated sender: dodji@seketeli.org) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id C4B7B60009; Mon, 27 Jul 2020 07:56:30 +0000 (UTC) Received: by localhost (Postfix, from userid 1000) id 58E52180090F; Mon, 27 Jul 2020 09:56:28 +0200 (CEST) From: Dodji Seketeli To: Giuliano Procida Cc: libabigail@sourceware.org, kernel-team@android.com, maennich@google.com Subject: Re: [PATCH 2/3] abg-ir.cc: Refactor operator== methods with helper Organization: Me, myself and I References: <20200708095315.948634-1-gprocida@google.com> <20200708095315.948634-3-gprocida@google.com> X-Operating-System: Red Hat Enterprise Linux Workstation 7.8 Beta X-URL: http://www.seketeli.net/~dodji Date: Mon, 27 Jul 2020 09:56:28 +0200 In-Reply-To: <20200708095315.948634-3-gprocida@google.com> (Giuliano Procida's message of "Wed, 8 Jul 2020 10:53:14 +0100") Message-ID: <87zh7lie6r.fsf@seketeli.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Spam-Status: No, score=-8.7 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP 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: Mailing list of the Libabigail project List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 Jul 2020 07:56:35 -0000 --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Giuliano Procida a =C3=A9crit: > Many of the operator=3D=3D definitions in this source file follow the same > pattern: > > - the address of the argument is dynamic_cast to type of 'this' > - naked canonical type pointers are compared, if both present > - the types are compared structurally with 'equals' > > In a couple of cases extra work is done to fetch the canonical type > of the definition of a declaration. > > This commit refactors all the common logic into a couple of templated > helper functions. > > There are no behavioural changes. > > * src/abg-ir.cc (equality_helper): Add an overloaded function > to perform the common actions needed for operator=3D=3D. The first > overload takes two extra canonical type pointer arguments > while the second obtains these from the types being compared. > (type_decl::operator=3D=3D): Call equality_helper to perform > canonical type pointer and 'equals' comparisons. > (scope_type_decl::operator=3D=3D): Likewise. > (qualified_type_def::operator=3D=3D): Likewise. > (pointer_type_def::operator=3D=3D): Likewise. > (reference_type_def::operator=3D=3D): Likewise. > (array_type_def::subrange_type::operator=3D=3D): Likewise. > (array_type_def::operator=3D=3D): Likewise. > (enum_type_decl::operator=3D=3D): Likewise. > (typedef_decl::operator=3D=3D): Likewise. > (function_type::operator=3D=3D): Likewise. > (class_or_union::operator=3D=3D): Likewise. > (class_decl::operator=3D=3D): Likewise. > (union_decl::operator=3D=3D): Likewise. > > Signed-off-by: Giuliano Procida > --- > src/abg-ir.cc | 104 ++++++++++++++------------------------------------ > 1 file changed, 29 insertions(+), 75 deletions(-) > > diff --git a/src/abg-ir.cc b/src/abg-ir.cc > index 41e2f00e..4b7e180d 100644 > --- a/src/abg-ir.cc > +++ b/src/abg-ir.cc > @@ -651,6 +651,22 @@ struct type_name_comp > {return operator()(type_base_sptr(l), type_base_sptr(r));} > }; // end struct type_name_comp >=20=20 > +template > +bool equality_helper(const T* lptr, const T* rptr, > + const type_base* lcanon, > + const type_base* rcanon) > +{ > + return lcanon && rcanon ? lcanon =3D=3D rcanon : equals(*lptr, *rptr, = 0); > +} > + > +template > +bool equality_helper(const T* lptr, const T* rptr) > +{ As done already throughout the code, the return type of functions should be on their own line, please. The name of the function should start on its own line as well. This makes searching for the definition of the function easy by typing a regular expression like "^equality_helper". Also, to make the code somewhat self documented, I try to have all function names contain a "verb". That forces us to give a name that tells what the function /does/. equality_helper is not quite useful in that respect. Essentially, what the function does is that it tries to compare the types canonically (i.e, using their canonical types) if possible. Otherwise, it falls back to structural comparison. So I'd rather call this something like try_canonical_compare. Last but not least, all function definitions should come documented, please. I have adjusted this accordingly and a patch that you'll find at the end of this message. > + return equality_helper(lptr, rptr, > + lptr->get_naked_canonical_type(), > + rptr->get_naked_canonical_type()); > +} > + > /// Getter of all types types sorted by their pretty representation. > /// > /// @return a sorted vector of all types sorted by their pretty > @@ -12690,11 +12706,7 @@ type_decl::operator=3D=3D(const decl_base& o) co= nst > const type_decl* other =3D dynamic_cast(&o); > if (!other) > return false; > - > - if (get_naked_canonical_type() && other->get_naked_canonical_type()) Something to keep in mind is that there are times when we need to debug some possibly tricky issues related to type comparison. Especially, we want to see why two types are (for instance) deemed different by libabigail. That is, we want to know why the types have different canonical types. A simple way to know this is to step in the debugger at this point and then make the debugger jump to the "return equals()" line below. That way, we force the code to take structural equality path in the debugger. We can thus step and see why the structural equality code decided that the two types are different (and thus that their canonical types are different). Yours changes in equality_helper are making this important debubbing much more difficult. So I have taken that into account in how I adjusted the try_canonical_compare function. > - return get_naked_canonical_type() =3D=3D other->get_naked_canonical_= type(); > - > - return equals(*this, *other, 0); > + return equality_helper(this, other); > } [...] > /// Return a copy of the pretty representation of the current @ref > @@ -19107,10 +19074,7 @@ class_or_union::operator=3D=3D(const decl_base& = other) const > other_canonical_type =3D > op->get_naked_definition_of_declaration()->get_naked_canonical_typ= e(); >=20=20 > - if (canonical_type && other_canonical_type) > - return canonical_type =3D=3D other_canonical_type; > - > - return equals(*this, *op, 0); > + return equality_helper(this, op, canonical_type, other_canonical_type); By massaging the code above this line, it's possible to call the second overload of equality_helper (just like what all the other spots are doing) and thus do away with the first overload of equality_helper. My amended patch does this. > } >=20=20 > /// Equality operator. > @@ -20961,10 +20925,7 @@ class_decl::operator=3D=3D(const decl_base& othe= r) const > other_canonical_type =3D > op->get_naked_definition_of_declaration()->get_naked_canonical_typ= e(); >=20=20 > - if (canonical_type && other_canonical_type) > - return canonical_type =3D=3D other_canonical_type; > - > - return equals(*this, *op, 0); > + return equality_helper(this, op, canonical_type, other_canonical_type); Likewise. > } [...] Here is the amended patch that I'd be for applying. I have also adjusted its commit log. Thanks. --=-=-= Content-Type: text/x-patch Content-Disposition: inline; filename=0001-abg-ir.cc-Refactor-operator-methods-with-helper-func.patch Content-Description: Amended patch >From cd06d651b8fca32a8385ff152bae972763e95b26 Mon Sep 17 00:00:00 2001 From: Giuliano Procida Date: Wed, 8 Jul 2020 10:53:14 +0100 Subject: [PATCH] abg-ir.cc: Refactor operator== methods with helper function Many of the operator== definitions in this source file follow the same pattern: - First, canonical comparison is attempted if canonical types are present. - Otherwise, the comparison is performed structurally using the 'equals' function. This commit refactors the common logic into a templated helper function named "try_canonical_compare". There are no behavioural changes. * src/abg-ir.cc (try_canonical_compare): New template function. (type_decl::operator==): Use it here. (scope_type_decl::operator==): Likewise. (qualified_type_def::operator==): Likewise. (pointer_type_def::operator==): Likewise. (reference_type_def::operator==): Likewise. (array_type_def::subrange_type::operator==): Likewise. (array_type_def::operator==): Likewise. (enum_type_decl::operator==): Likewise. (typedef_decl::operator==): Likewise. (function_type::operator==): Likewise. (class_or_union::operator==): Likewise. (class_decl::operator==): Likewise. (union_decl::operator==): Likewise. Signed-off-by: Giuliano Procida Signed-off-by: Dodji Seketeli --- src/abg-ir.cc | 166 +++++++++++++++++++++------------------------------------- 1 file changed, 59 insertions(+), 107 deletions(-) diff --git a/src/abg-ir.cc b/src/abg-ir.cc index f6eb285..ac837da 100644 --- a/src/abg-ir.cc +++ b/src/abg-ir.cc @@ -651,6 +651,25 @@ struct type_name_comp {return operator()(type_base_sptr(l), type_base_sptr(r));} }; // end struct type_name_comp +/// Compare two types by comparing their canonical types if present. +/// +/// If the canonical types are not present (because the types have not +/// yet been canonicalized, for instance) then the types are compared +/// structurally. +/// +/// @param l the first type to take into account in the comparison. +/// +/// @param r the second type to take into account in the comparison. +template +bool +try_canonical_compare(const T *l, const T *r) +{ + if (const type_base *lc = l->get_naked_canonical_type()) + if (const type_base *rc = r->get_naked_canonical_type()) + return lc == rc; + return equals(*l, *r, 0); +} + /// Getter of all types types sorted by their pretty representation. /// /// @return a sorted vector of all types sorted by their pretty @@ -12904,11 +12923,7 @@ type_decl::operator==(const decl_base& o) const const type_decl* other = dynamic_cast(&o); if (!other) return false; - - if (get_naked_canonical_type() && other->get_naked_canonical_type()) - return get_naked_canonical_type() == other->get_naked_canonical_type(); - - return equals(*this, *other, 0); + return try_canonical_compare(this, other); } /// Return true if both types equals. @@ -13085,11 +13100,7 @@ scope_type_decl::operator==(const decl_base& o) const const scope_type_decl* other = dynamic_cast(&o); if (!other) return false; - - if (get_naked_canonical_type() && other->get_naked_canonical_type()) - return get_naked_canonical_type() == other->get_naked_canonical_type(); - - return equals(*this, *other, 0); + return try_canonical_compare(this, other); } /// Equality operator between two scope_type_decl. @@ -13453,11 +13464,7 @@ qualified_type_def::operator==(const decl_base& o) const dynamic_cast(&o); if (!other) return false; - - if (get_naked_canonical_type() && other->get_naked_canonical_type()) - return get_naked_canonical_type() == other->get_naked_canonical_type(); - - return equals(*this, *other, 0); + return try_canonical_compare(this, other); } /// Equality operator for qualified types. @@ -13824,14 +13831,7 @@ pointer_type_def::operator==(const decl_base& o) const const pointer_type_def* other = is_pointer_type(&o); if (!other) return false; - - type_base* canonical_type = get_naked_canonical_type(); - type_base* other_canonical_type = other->get_naked_canonical_type(); - - if (canonical_type && other_canonical_type) - return canonical_type == other_canonical_type; - - return equals(*this, *other, 0); + return try_canonical_compare(this, other); } /// Return true iff both instances of pointer_type_def are equal. @@ -14135,14 +14135,7 @@ reference_type_def::operator==(const decl_base& o) const dynamic_cast(&o); if (!other) return false; - - type_base* canonical_type = get_naked_canonical_type(); - type_base* other_canonical_type = other->get_naked_canonical_type(); - - if (canonical_type && other_canonical_type) - return canonical_type == other_canonical_type; - - return equals(*this, *other, 0); + return try_canonical_compare(this, other); } /// Equality operator of the @ref reference_type_def type. @@ -14684,11 +14677,7 @@ array_type_def::subrange_type::operator==(const decl_base& o) const dynamic_cast(&o); if (!other) return false; - - if (get_naked_canonical_type() && other->get_naked_canonical_type()) - return get_naked_canonical_type() == other->get_naked_canonical_type(); - - return equals(*this, *other, 0); + return try_canonical_compare(this, other); } /// Equality operator. @@ -15006,11 +14995,7 @@ array_type_def::operator==(const decl_base& o) const dynamic_cast(&o); if (!other) return false; - - if (get_naked_canonical_type() && other->get_naked_canonical_type()) - return get_naked_canonical_type() == other->get_naked_canonical_type(); - - return equals(*this, *other, 0); + return try_canonical_compare(this, other); } bool @@ -15485,11 +15470,7 @@ enum_type_decl::operator==(const decl_base& o) const const enum_type_decl* op = dynamic_cast(&o); if (!op) return false; - - if (get_naked_canonical_type() && op->get_naked_canonical_type()) - return get_naked_canonical_type() == op->get_naked_canonical_type(); - - return equals(*this, *op, 0); + return try_canonical_compare(this, op); } /// Equality operator. @@ -15839,11 +15820,7 @@ typedef_decl::operator==(const decl_base& o) const const typedef_decl* other = dynamic_cast(&o); if (!other) return false; - - if (get_naked_canonical_type() && other->get_naked_canonical_type()) - return get_naked_canonical_type() == other->get_naked_canonical_type(); - - return equals(*this, *other, 0); + return try_canonical_compare(this, other); } /// Equality operator @@ -16934,14 +16911,7 @@ function_type::operator==(const type_base& other) const const function_type* o = dynamic_cast(&other); if (!o) return false; - - type_base* canonical_type = get_naked_canonical_type(); - type_base* other_canonical_type = other.get_naked_canonical_type(); - - if (canonical_type && other_canonical_type) - return canonical_type == other_canonical_type; - - return equals(*this, *o, 0); + return try_canonical_compare(this, o); } /// Return a copy of the pretty representation of the current @ref @@ -19201,29 +19171,22 @@ class_or_union::operator==(const decl_base& other) const if (!op) return false; - type_base *canonical_type = get_naked_canonical_type(), - *other_canonical_type = op->get_naked_canonical_type(); - - // If this is a declaration only class with no canonical class, use - // the canonical type of the definition, if any. - if (!canonical_type - && get_is_declaration_only() - && get_naked_definition_of_declaration()) - canonical_type = is_class_or_union_type - (get_naked_definition_of_declaration())->get_naked_canonical_type(); + // If this is a decl-only type (and thus with no canonical type), + // use the canonical type of the definition, if any. + const class_or_union *l = 0; + if (get_is_declaration_only()) + l = dynamic_cast(get_naked_definition_of_declaration()); + if (l == 0) + l = this; // Likewise for the other class. - if (!other_canonical_type - && op->get_is_declaration_only() - && op->get_naked_definition_of_declaration()) - other_canonical_type = - is_class_or_union_type - (op->get_naked_definition_of_declaration())->get_naked_canonical_type(); + const class_or_union *r = 0; + if (op->get_is_declaration_only()) + r = dynamic_cast(op->get_naked_definition_of_declaration()); + if (r == 0) + r = op; - if (canonical_type && other_canonical_type) - return canonical_type == other_canonical_type; - - return equals(*this, *op, 0); + return try_canonical_compare(l, r); } /// Equality operator. @@ -21032,30 +20995,26 @@ class_decl::operator==(const decl_base& other) const if (!op) return false; - type_base *canonical_type = get_naked_canonical_type(), - *other_canonical_type = op->get_naked_canonical_type(); + // If this is a decl-only type (and thus with no canonical type), + // use the canonical type of the definition, if any. + const class_decl *l = 0; + if (get_is_declaration_only()) + l = dynamic_cast(get_naked_definition_of_declaration()); + if (l == 0) + l = this; - // If this is a declaration only class with no canonical class, use - // the canonical type of the definition, if any. - if (!canonical_type - && get_is_declaration_only() - && get_naked_definition_of_declaration()) - canonical_type = - is_class_type - (get_naked_definition_of_declaration())->get_naked_canonical_type(); + ABG_ASSERT(l); - // Likewise for the other class. - if (!other_canonical_type - && op->get_is_declaration_only() - && op->get_naked_definition_of_declaration()) - other_canonical_type = - is_class_type - (op->get_naked_definition_of_declaration())->get_naked_canonical_type(); + // Likewise for the other type. + const class_decl *r = 0; + if (op->get_is_declaration_only()) + r = dynamic_cast(op->get_naked_definition_of_declaration()); + if (r == 0) + r = op; - if (canonical_type && other_canonical_type) - return canonical_type == other_canonical_type; + ABG_ASSERT(r); - return equals(*this, *op, 0); + return try_canonical_compare(l, r); } /// Equality operator for class_decl. @@ -21836,14 +21795,7 @@ union_decl::operator==(const decl_base& other) const const union_decl* op = dynamic_cast(&other); if (!op) return false; - - type_base *canonical_type = get_naked_canonical_type(), - *other_canonical_type = op->get_naked_canonical_type(); - - if (canonical_type && other_canonical_type) - return canonical_type == other_canonical_type; - - return equals(*this, *op, 0); + return try_canonical_compare(this, op); } /// Equality operator for union_decl. -- 1.8.3.1 --=-=-= Content-Type: text/plain -- Dodji --=-=-=--