[PATCH] Bug 22075 data_member_diff_comp comparison functor isn't a total ordering.

[Mark Wielaard]

Hi Dodji,

On Wed, 2017-09-13 at 14:23 +0200, Dodji Seketeli wrote:
> Mark Wielaard <mark@klomp.org> a écrit:
> >  /// A comparison functor to compare two instances of @ref var_diff
> >  /// that represent changed data members based on the offset of their
> > -/// initial value.
> > +/// initial value, or if equal based on the offset of their second
> > +/// instance, of if those are also equal, based on their name.
> >  struct data_member_diff_comp
> >  {
> >    /// @param f the first change to data member to take into account
> > @@ -6749,7 +6750,29 @@ struct data_member_diff_comp
> >      assert(is_data_member(first_dm));
> >      assert(is_data_member(second_dm));
> >  
> > -    return get_data_member_offset(first_dm) < get_data_member_offset(second_dm);
> > +    size_t off1 = get_data_member_offset(first_dm);
> > +    size_t off2 = get_data_member_offset(second_dm);
> > +    if (off1 != off2)
> > +      return off1 < off2;
> > +
> > +    first_dm = f->second_var();
> > +    second_dm = s->second_var();
> > +
> > +    assert(is_data_member(first_dm));
> > +    assert(is_data_member(second_dm));
> > +
> > +    off1 = get_data_member_offset(first_dm);
> > +    off2 = get_data_member_offset(second_dm);
> > +
> > +    if (off1 != off2)
> > +      return off1 < off2;
> 
> Just so we are clear, the diff nodes (f and s) represent a change
> from
> an initial data member to a new data member.  What you are comparing
> here are the offsets of the new data members.

Yes. What we want is to see whether we should show the first or second
diff note first. It doesn't really matter which one we pick. But we
want it to be consistent and stable (given two diff nodes, we always
want to pick the same one).

> And then ...
> 
> > +
> > +    string name1 = first_dm->get_qualified_name();
> > +    string name2 = second_dm->get_qualified_name();
> > +
> > +    assert (name1 != name2);
> > +
> > +    return name1 < name2;
> 
> ... here, you are comparing the qualified names of these new data
> members.
> 
> I think it'd be more natural to compare the qualified names of the
> *initial* data members first (when their offset is equal) and then
> compare the offset and qualified names of the new data members.

I did the offsets first because I assumed that was the
quickest/cheapest comparison to compare. And I assumed that if the
offsets are the same, then it doesn't matter which names (firsts or
seconds var) you compare. You get a complete ordering whichever you
pick (after comparing the offsets).

So I think your revised ordering of the comparisons is correct.
But I do have one comment. You got:

> +    // The two offsets of the initial data members are the same.  So
> +    // lets compare the qualified name of these initial data members.
> +
> +    string name1 = first_dm->get_qualified_name();
> +    string name2 = second_dm->get_qualified_name();
> +
> +    assert (name1 != name2);
> +
> +    if (name1 != name2)
> +      return name1 < name2;
> [...]

If the "if" statement wasn't true, the assert would trigger.
So with that we don't need to compare the second var offsets or name at
all to get a complete ordering (the rest is now just dead code)?

> This would result in the slightly modified patch below.  Would this
> one pass make distcheck in the autobuilder environment?

Yes, the modified patch has a clean build on fedora-s390x, debian-i386
and fedora-x86_64. The debian-amd64 and centos-x86_64 ones still fail,
but for unrelated (existing) reasons.

>   If yes, you can
> just commit that one, or I can do it for you if you like.

Please commit it if you are happy with it.
I don't believe I have commit access.

Cheers,

Mark