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

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

[Mark Wielaard]

Make data_member_diff_comp comparison functor a total ordering.
The initial value offset can be similar, in that case order based
on the offset of the second instance, or if those are also equal
order based on the qualified name.

This makes sure that offset change reports have a stable ordering.
Makes the runtestdiffpkg testcase succeeds on debian-i386.

	* src/abg-comparison.cc (data_member_diff_comp): Make comparison
	functor a total ordering.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 src/abg-comparison.cc | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index 8747e0c..70221e6 100644
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -6731,7 +6731,8 @@ sort_string_base_diff_sptr_map(const string_base_diff_sptr_map& map,
 
 /// 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;
+
+    string name1 = first_dm->get_qualified_name();
+    string name2 = second_dm->get_qualified_name();
+
+    assert (name1 != name2);
+
+    return name1 < name2;
   }
 }; // end struct var_diff_comp
 
-- 
1.8.3.1

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

[Dodji Seketeli]

[-- Attachment #1: Type: text/plain, Size: 2762 bytes --]

Hello Mark,

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

> Make data_member_diff_comp comparison functor a total ordering.
> The initial value offset can be similar, in that case order based
> on the offset of the second instance, or if those are also equal
> order based on the qualified name.
>
> This makes sure that offset change reports have a stable ordering.
> Makes the runtestdiffpkg testcase succeeds on debian-i386.
>
> 	* src/abg-comparison.cc (data_member_diff_comp): Make comparison
> 	functor a total ordering.

Thanks, this is really useful.

I just have some light comments below.

[...]

> diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc

[...]


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

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.

This would result in the slightly modified patch below.  Would this one
pass make distcheck in the autobuilder environment?  If yes, you can
just commit that one, or I can do it for you if you like.

Thanks a lot and cheers!


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Tentative patch --]
[-- Type: text/x-patch, Size: 3188 bytes --]

From 18bfcd6af0b48b8b41ede85f28668245161500e2 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mark@klomp.org>
Date: Mon, 11 Sep 2017 22:50:22 +0200
Subject: [PATCH] Bug 22075 - data_member_diff_comp comparison functor isn't a
 total ordering

Make data_member_diff_comp comparison functor a total ordering.
The initial value offset can be similar, in that case order based
on the offset of the second instance, or if those are also equal
order based on the qualified name.

This makes sure that offset change reports have a stable ordering.
Makes the runtestdiffpkg testcase succeeds on debian-i386.

	* src/abg-comparison.cc (data_member_diff_comp): Make comparison
	functor a total ordering.

Signed-off-by: Mark Wielaard <mark@klomp.org>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 src/abg-comparison.cc | 49 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 46 insertions(+), 3 deletions(-)

diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index 8747e0c..86d8cb0 100644
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -6730,8 +6730,10 @@ sort_string_base_diff_sptr_map(const string_base_diff_sptr_map& map,
 }
 
 /// A comparison functor to compare two instances of @ref var_diff
-/// that represent changed data members based on the offset of their
-/// initial value.
+/// that represent changed data members based on the offset of the
+/// initial data members, or if equal, based on their qualified name.
+/// If equal again, then the offset and qualified name of the of the
+/// new data members are considered.
 struct data_member_diff_comp
 {
   /// @param f the first change to data member to take into account
@@ -6749,7 +6751,48 @@ 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;
+
+    // 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;
+
+    // The offsets and the qualified names of the initial data members
+    // are the same.  Let's now compare the offsets of the *new* data
+    // members.
+
+    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;
+
+    // The offsets of the new data members are the same, dang!  Let's
+    // compare the qualified names of these new data members then.
+
+    name1 = first_dm->get_qualified_name();
+    name2 = second_dm->get_qualified_name();
+
+    assert (name1 != name2);
+
+    return name1 < name2;
   }
 }; // end struct var_diff_comp
 
-- 
1.8.3.1


[-- Attachment #3: Type: text/plain, Size: 13 bytes --]


-- 
		Dodji

Re: [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

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

[Dodji Seketeli]

[-- Attachment #1: Type: text/plain, Size: 884 bytes --]

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.

Below is the updated patch.

Cheers,


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Updated patch --]
[-- Type: text/x-patch, Size: 3120 bytes --]

From 2f624d5fd998c7cd9d264631508514e3f19e808e Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mark@klomp.org>
Date: Mon, 11 Sep 2017 22:50:22 +0200
Subject: [PATCH] Bug 22075 - data_member_diff_comp comparison functor isn't a
 total ordering

Make data_member_diff_comp comparison functor a total ordering.
The initial value offset can be similar, in that case order based
on the offset of the second instance, or if those are also equal
order based on the qualified name.

This makes sure that offset change reports have a stable ordering.
Makes the runtestdiffpkg testcase succeeds on debian-i386.

	* src/abg-comparison.cc (data_member_diff_comp): Make comparison
	functor a total ordering.

Signed-off-by: Mark Wielaard <mark@klomp.org>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 src/abg-comparison.cc | 45 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index 8747e0c..ac05309 100644
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -6730,8 +6730,10 @@ sort_string_base_diff_sptr_map(const string_base_diff_sptr_map& map,
 }
 
 /// A comparison functor to compare two instances of @ref var_diff
-/// that represent changed data members based on the offset of their
-/// initial value.
+/// that represent changed data members based on the offset of the
+/// initial data members, or if equal, based on their qualified name.
+/// If equal again, then the offset and qualified name of the of the
+/// new data members are considered.
 struct data_member_diff_comp
 {
   /// @param f the first change to data member to take into account
@@ -6749,7 +6751,44 @@ 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;
+
+    // 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();
+
+    if (name1 != name2)
+      return name1 < name2;
+
+    // The offsets and the qualified names of the initial data members
+    // are the same.  Let's now compare the offsets of the *new* data
+    // members.
+
+    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;
+
+    // The offsets of the new data members are the same, dang!  Let's
+    // compare the qualified names of these new data members then.
+
+    name1 = first_dm->get_qualified_name();
+    name2 = second_dm->get_qualified_name();
+
+    return name1 < name2;
   }
 }; // end struct var_diff_comp
 
-- 
1.8.3.1


[-- Attachment #3: Type: text/plain, Size: 13 bytes --]


-- 
		Dodji

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

[Mark Wielaard]

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

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

[Dodji Seketeli]

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

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

[Mark Wielaard]

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

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

[Dodji Seketeli]

[-- Attachment #1: Type: text/plain, Size: 179 bytes --]

Hey Mark,

OK, so I have committed your patch, slightly edited, to the master
branch.

Below is what hit the repository in the end.

Thanks again for looking into this.

Cheers,


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Final patch --]
[-- Type: text/x-patch, Size: 3278 bytes --]

From 60a4743af492ba1f00adece30a9c5e34d5fd1b42 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mark@klomp.org>
Date: Mon, 11 Sep 2017 22:50:22 +0200
Subject: [PATCH 1/3] Bug 22075 - data_member_diff_comp forgets data members
 names

This patch makes the data_member_diff_comp comparison functor consider
all the properties local to the data member: that is, its offset and
its name.

It used to only take the offset into account.

This makes sure that offset change reports have a stable ordering and
thus makes the runtestdiffpkg testcase succeeds on debian-i386.

	* src/abg-comparison.cc (data_member_diff_comp): Make the
	comparison take the qualified name of the data member into
	account.  Also, if the initial offset and qualified names of the
	data members of the diff nodes are equal, consider the offset and
	qualified names of the new data members.

Signed-off-by: Mark Wielaard <mark@klomp.org>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 src/abg-comparison.cc | 45 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index 8747e0c..9580509 100644
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -6730,8 +6730,10 @@ sort_string_base_diff_sptr_map(const string_base_diff_sptr_map& map,
 }
 
 /// A comparison functor to compare two instances of @ref var_diff
-/// that represent changed data members based on the offset of their
-/// initial value.
+/// that represent changed data members based on the offset of the
+/// initial data members, or if equal, based on their qualified name.
+/// If equal again, then the offset and qualified name of the new data
+/// members are considered.
 struct data_member_diff_comp
 {
   /// @param f the first change to data member to take into account
@@ -6749,7 +6751,44 @@ 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;
+
+    // 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();
+
+    if (name1 != name2)
+      return name1 < name2;
+
+    // The offsets and the qualified names of the initial data members
+    // are the same.  Let's now compare the offsets of the *new* data
+    // members.
+
+    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;
+
+    // The offsets of the new data members are the same, dang!  Let's
+    // compare the qualified names of these new data members then.
+
+    name1 = first_dm->get_qualified_name();
+    name2 = second_dm->get_qualified_name();
+
+    return name1 < name2;
   }
 }; // end struct var_diff_comp
 
-- 
1.8.3.1


[-- Attachment #3: Type: text/plain, Size: 13 bytes --]


-- 
		Dodji

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

[Mark Wielaard]

On Wed, 2017-09-13 at 15:36 +0200, Mark Wielaard wrote:
> 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?

Answering myself. Since I am clearly confused :)
When the first name comparison is done, we have only compared the
initial offsets, so the names can still be identical. That just implies
the change offsets will be different.

I am still slightly confused why the second/last name comparison will
ever be reached. And if it can be reached why it must always be
different and cannot be equal. But I might not fully understand what
the change nodes represent.

Cheers,

Mark