From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-il1-x142.google.com (mail-il1-x142.google.com [IPv6:2607:f8b0:4864:20::142]) by sourceware.org (Postfix) with ESMTPS id 2B37F3858D35 for ; Mon, 27 Jul 2020 10:37:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 2B37F3858D35 Received: by mail-il1-x142.google.com with SMTP id b18so7427739ilo.12 for ; Mon, 27 Jul 2020 03:37:08 -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:content-transfer-encoding; bh=mMvjz0hbny3LHxzar5zZy2EOrajfKe3SmhumGDmNSyk=; b=lyB+TCtaSwX6rQRu3gbqLVMVProVtScq9+WZedvNhlxYzu6qlMRYVpdU8v9urDH/wq WHy9l3zae8+iX7SeQm95KloSTke+FiSBIbXUaIoHA8TUJ4LGSxzomcTZO7KHEpjzvGgE 0SZlPIqffAqiui6UgUBhvedxfDoJbWgJe6XFKiyyRcfPiCCOaer22nsax3Mv6lqXUGqH jACYo5gd532ZnrM8//kkVwi9B6Z2J3QG1+R22fhu/YmtM6f5iv7vwLlP1PyI64P+goC0 jcCasA+WNhk5ljGU6lXXftEb4oFFjdXL0HVQus+Lj5+WygcV1Trwo0d8qS0jwLQXU7q/ I0Dw== X-Gm-Message-State: AOAM532qHtYPkHKvBZ/u5Y2gigCS/gOnBt31lTVVFn4uun1Nwgd1uaj4 ttwR65mUXMuIFZINS+6h8COo3B2Z5TzEsv0y9bMd+3hN X-Google-Smtp-Source: ABdhPJxZPKcA311LOq8WG91CXQuQenQFelXsCt9w5dcXzbF0aJiPjOGGeBYfvbJIwqV9PgAsAzKDuUwa2F8XapDqmqA= X-Received: by 2002:a92:9108:: with SMTP id t8mr24281636ild.170.1595846227353; Mon, 27 Jul 2020 03:37:07 -0700 (PDT) MIME-Version: 1.0 References: <20200708095315.948634-1-gprocida@google.com> <20200708095315.948634-3-gprocida@google.com> <87zh7lie6r.fsf@seketeli.org> In-Reply-To: <87zh7lie6r.fsf@seketeli.org> From: Giuliano Procida Date: Mon, 27 Jul 2020 11:36:30 +0100 Message-ID: Subject: Re: [PATCH 2/3] abg-ir.cc: Refactor operator== methods with helper To: Dodji Seketeli Cc: libabigail@sourceware.org, kernel-team@android.com, =?UTF-8?Q?Matthias_M=C3=A4nnich?= Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-29.1 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, 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: Mailing list of the Libabigail project List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 Jul 2020 10:37:09 -0000 Hi. On Mon, 27 Jul 2020 at 08:56, Dodji Seketeli wrote: > > Giuliano Procida a =C3=A9crit: > > > Many of the operator=3D=3D definitions in this source file follow the s= ame > > 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 firs= t > > 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 > > > > +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. Of course. Sorry. > 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. Yes, the name wasn't ideal. > Last but not least, all function definitions should come documented, > please. Oops. > 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) = const > > 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. That's a shame. One motivation was to make it easier to perform bulk checks against canonical and structural equality logic. I see your tweaks to a couple of the functions enable a uniform calling path into the helper, allowing the removal of the second overload. > > - return get_naked_canonical_type() =3D=3D other->get_naked_canonica= l_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_t= ype(); > > > > - 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_typ= e); > > 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. > > > } > > > > /// Equality operator. > > @@ -20961,10 +20925,7 @@ class_decl::operator=3D=3D(const decl_base& ot= her) const > > other_canonical_type =3D > > op->get_naked_definition_of_declaration()->get_naked_canonical_t= ype(); > > > > - 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_typ= e); > > Likewise. > > > } > > [...] > > Here is the amended patch that I'd be for applying. I have also > adjusted its commit log. Thanks. > It looks good to me. Giuliano. > > -- > Dodji