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

[Dodji Seketeli <dodji@seketeli.org>]

Mark Wielaard <mark@klomp.org> a écrit:


> This might be nitpicking, but why isn't the code after the first name
> comparison dead? Given two change nodes, if the initial offsets are the
> same and the secondary offsets are the same (meaning they moved
> similarly), then how can the names also be the same?

There could be another property of the data member that changed.  There
can be many many of these.  Any property about the type of the data
member could have changed, for instance.  There can be tons of them.
This function is not the place to look at the properties of the type of
the data member.  It's just meant to look at properties of the data
member.

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.

There are other steps that are later used to sort of the changes of the
type of the data structure that are emitted later in the pipeline.


> Or is it allowed for the comparison operator to be called
> with identical change nodes?

Yes, identical as far the properties of the data members are concerned.
And then, there is another sorting that will take place for the
properties of the sub-components of the data member (like its type).

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

Cheers,

-- 
		Dodji