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 15:24 +0200, Dodji Seketeli wrote:
> Mark Wielaard <mark@klomp.org> a écrit:
> 
> [...]
> 
> > > +    // 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)?
> 
> Right.  I wanted to remove the assert because the two names can very
> well be equal.  I am removing the other similar assert later down the
> patch as well.

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? 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? Or is it allowed for the comparison operator to be called
with identical change nodes? 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).

Also, I would keep the last assert. It should 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.

Cheers,

Mark