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

[Mark Wielaard <mark@klomp.org>]

On Wed, 2017-09-13 at 16:33 +0200, Dodji Seketeli wrote:
> If all the properties of the data members are equal then the function
> should return false.
> 
> > Wouldn't that mean that the first and second change node are
> > identical? If they aren't, then isn't there another attribute we
> > can check to create a total ordering?
> 
> Actually, thinking about this deeper, this function is not meant to
> define a total ordering on *all* the properties of data member and
> its type.  It's meant to define a total ordering only for the
> properties that really belong directly to the data member, namely,
> its offset and its name.

OK, and the new offset (and possible new name).
Because that are the only properties used in the diff report.
The other properties aren't used/printed there.

> >  But if so, then the comparison operator shouldn't be allowed to
> > return a boolean (because it cannot answer which change node comes
> > first for two identical nodes).
> 
> I don't think so.  The boolean is just to say if the first operand is
> less than the second.
> 
> > Also, I would keep the last assert. It should hold.
> 
> I don't think so.  If for instance the diff nodes we are looking at
> are just about changes in the types of the data members without
> impacting neither their name nor their offset then the assert
> wouldn't hold.
> 
> > But if it doesn't then the return name1 < name2 would introduce the
> > same issue again (the ordering would not be total). Better to catch
> > that early.
> 
> I hope I have explained why the fact that the order is not strictly
> total is not an issue.

Right. I now see it defines a weak ordering, not a total ordering. If
all relevant properties are the same, it doesn't matter which one comes
"first" since the diff report will look the same anyway.

I was slightly confused because I don't fully understand the
relationship between the names. I believe there is no example
(currently) in the tests where there is a report where the offsets stay
the same, but only the name(s) change.

So the bug was that not all relevant properties were taken into account
to determine the ordering. Leading to reporting a change diff in
different order depending on implementation/architectural details. Now
that all relevant properties are compared then diff reports are
consistent between all arches.

Thanks,

Mark