[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Bug 22075 data_member_diff_comp comparison functor isn't a total ordering.
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!
>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
--
Dodji