Re: [RFC PATCH 1/1] Fix decl_base comparison function

[Dodji Seketeli <dodji@seketeli.org>]

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

[...]

> diff --git a/src/abg-ir.cc b/src/abg-ir.cc
> index 1fe6f499..aa2a56fa 100644
> --- a/src/abg-ir.cc
> +++ b/src/abg-ir.cc
> @@ -4067,26 +4067,33 @@ equals(const decl_base& l, const decl_base& r, change_kind* k)
>    /// Otherwise, let's just compare their name, the obvious way.
>    /// That's the fast path because in that case the names are
>    /// interned_string and comparing them is much faster.
> -  bool decls_are_different = (ln != rn);
> -  if (decls_are_different
> +  bool decls_are_same = (ln == rn);
> +  if (!decls_are_same
>        && l.get_is_anonymous()
>        && !l.get_has_anonymous_parent()
>        && r.get_is_anonymous()
>        && !r.get_has_anonymous_parent())
> -    // Both decls are anonymous and their scope are *NOT* anonymous.
> -    // So we consider the decls to have equivalent names (both
> -    // anonymous, remember).  We are still in the fast path here.
> -    decls_are_different = false;
> +    {
> +      // Both decls are anonymous and their scope are *NOT* anonymous.
> +      // So we consider the decls to have equivalent names (both
> +      // anonymous, remember).  We are still in the fast path here.
> +      //
> +      // TODO: Don't conflate anonymous structs, unions and enums?

Yes, we want to compare decls here, irrespective (as much as possible)
of the kind of types they are.  That is precisely the point of this
function.  The specifics of structs, unions and enums are dealt with in
the overloads of the equals functions that are specific to those types.
Those specific overloads use this one, precisely to compare the generic
"decl-related-part" of those types.  That generic part has to do with
the "naming" of those entities.

> +      //
> +      // TODO: Should we really be conflating all foo1::..::fooM::anon
> +      // with all bar1::..::barN::anon?

The reasoning here is that two declarations entities that are anonymous
can not be named, by definition.  And that is irrespective of their
(naming) context.  So, for naming purposes, we can't say for sure that
foo1::..::fooM::anon and bar1::..::barN::anon are different just based
on their name (or the name of their context for that matter).

To tell if these two entities are different, we'd have to look at
something else but their name.  So for now, we assume they are equal.
later down in the function, other properties (related to declarations in
the generic sense) are looked at to try to determine if they are equal.

So I am removing these two TODO comments.

> +      decls_are_same = true;
> +    }
>  
> -  if (decls_are_different
> +  if (!decls_are_same
>        && l.get_has_anonymous_parent()
>        && r.get_has_anonymous_parent())
>      // This is the slow path as we are comparing the decl qualified
>      // names component by component, properly handling anonymous
>      // scopes.
> -    decls_are_different = tools_utils::decl_names_equal(ln, rn);
> +    decls_are_same = tools_utils::decl_names_equal(ln, rn);
>  
> -  if (decls_are_different)
> +  if (!decls_are_same)
>      {
>        result = false;
>        if (k)

[...]


>
> 	* src/abg-ir.cc (equals): In the decl_base overload, note that
> 	the value returned by decl_names_equal should be negated and
> 	replace decls_are_different with decls_are_same, negating all
> 	occurrences.
> 	* tests/data/test-diff-dwarf/PR25058-liblttng-ctl-report-1.txt:
> 	Update tests, removing some spurious anonymous union name change.
> 	* tests/data/test-diff-filter/test33-report-0.txt: Diff now
> 	completely empty.
> 	* tests/data/test-diff-pkg/elfutils-libs-0.170-4.el7.x86_64-multiple-sym-vers-report-0.txt:
> 	3 functions previously considered to have harmless changes are
> 	now deemed to have no changes.
> 	* tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt:
> 	1 struct RedStore data member previously considered to have
> 	harmless changes is now deemed to have no changes.
> 	* tests/data/test-read-dwarf/test9-pr18818-clang.so.abi:
> 	One instance of an anonymous struct removed and a typedef
> 	repointed at another existing instance; many type ids
> 	renumbered.
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>

Applied to master with the change above and after adjusting the commit
log.

Thanks!

Cheers,



-- 
		Dodji