Re: [PATCH 2/3] abg-ir.cc: Refactor operator== methods with helper

[Giuliano Procida <gprocida@google.com>]

Hi.

On Mon, 27 Jul 2020 at 08:56, Dodji Seketeli <dodji@seketeli.org> wrote:
>
> Giuliano Procida <gprocida@google.com> a écrit:
>
> > Many of the operator== 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==. The first
> >       overload takes two extra canonical type pointer arguments
> >       while the second obtains these from the types being compared.
> >       (type_decl::operator==): Call equality_helper to perform
> >       canonical type pointer and 'equals' comparisons.
> >       (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 <gprocida@google.com>
> > ---
> >  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<typename T>
> > +bool equality_helper(const T* lptr, const T* rptr,
> > +                  const type_base* lcanon,
> > +                  const type_base* rcanon)
> > +{
> > +  return lcanon && rcanon ? lcanon == rcanon : equals(*lptr, *rptr, 0);
> > +}
> > +
> > +template<typename T>
> > +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==(const decl_base& o) const
> >    const type_decl* other = dynamic_cast<const type_decl*>(&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() == 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==(const decl_base& other) const
> >      other_canonical_type =
> >        op->get_naked_definition_of_declaration()->get_naked_canonical_type();
> >
> > -  if (canonical_type && other_canonical_type)
> > -    return canonical_type == 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.
>
> >  }
> >
> >  /// Equality operator.
> > @@ -20961,10 +20925,7 @@ class_decl::operator==(const decl_base& other) const
> >      other_canonical_type =
> >        op->get_naked_definition_of_declaration()->get_naked_canonical_type();
> >
> > -  if (canonical_type && other_canonical_type)
> > -    return canonical_type == 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.
>

It looks good to me.

Giuliano.

>
> --
>                 Dodji