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

  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(-)
  

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.

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==(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.

> -    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.
  

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
  

Giuliano Procida <gprocida@google.com> a écrit:

[...]

> It looks good to me.

Applied to master, thanks!