* [PATCH 1/2] Improve readability of represent helper function.
@ 2020-04-08 12:20 Giuliano Procida
2020-04-08 12:20 ` [PATCH 2/2] abidiff: Document and refresh tests Giuliano Procida
2020-04-08 12:47 ` [PATCH 1/2] Improve readability of represent helper function Matthias Maennich
0 siblings, 2 replies; 10+ messages in thread
From: Giuliano Procida @ 2020-04-08 12:20 UTC (permalink / raw)
To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich
This change:
- makes local variables const where possible
- gives local variables more descriptive names
- factors out some more expressions as local variables
- simplifies the logic around anonymous data members
There are no behavioural changes.
* src/abg-reporter-priv.cc (represent): In the var_diff_sptr
overload, rename pretty_representation to
o_pretty_representation, introduce n_pretty_representation
where needed and replace the duplicate tr1 and tr2 with these;
rename all other variables foo1 and foo2 to o_foo and n_foo
respectively, using _type instead of _t; introduce o_anon and
n_anon and use them to make the local variable
is_strict_anonymous_data_member_change const, make ABG_ASSERT
in anonymous data member handling more obvious in the case
where anonymity has changed and allow simplification of
formatting in this case; move declarations of const local
variables above those of the non-const, state-tracking,
variables.
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
src/abg-reporter-priv.cc | 117 +++++++++++++++++----------------------
1 file changed, 50 insertions(+), 67 deletions(-)
diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc
index 5fb5fbc5..a46c0e9d 100644
--- a/src/abg-reporter-priv.cc
+++ b/src/abg-reporter-priv.cc
@@ -409,14 +409,21 @@ represent(const var_diff_sptr &diff,
if (!ctxt->get_reporter()->diff_to_be_reported(diff.get()))
return;
- var_decl_sptr o = diff->first_var();
- var_decl_sptr n = diff->second_var();
- string name1 = o->get_qualified_name();
- string name2 = n->get_qualified_name();
- uint64_t size1 = get_var_size_in_bits(o);
- uint64_t size2 = get_var_size_in_bits(n);
- uint64_t offset1 = get_data_member_offset(o);
- uint64_t offset2 = get_data_member_offset(n);
+ const var_decl_sptr o = diff->first_var();
+ const var_decl_sptr n = diff->second_var();
+ const bool o_anon = !!is_anonymous_data_member(o);
+ const bool n_anon = !!is_anonymous_data_member(n);
+ const bool is_strict_anonymous_data_member_change = o_anon && n_anon;
+ const string o_name = o->get_qualified_name();
+ const string n_name = n->get_qualified_name();
+ const uint64_t o_size = get_var_size_in_bits(o);
+ const uint64_t n_size = get_var_size_in_bits(n);
+ const uint64_t o_offset = get_data_member_offset(o);
+ const uint64_t n_offset = get_data_member_offset(n);
+ const string o_pretty_representation = o->get_pretty_representation();
+
+ const bool show_size_offset_changes = ctxt->get_allowed_category()
+ & SIZE_OR_OFFSET_CHANGE_CATEGORY;
// Has the main diff text been output?
bool emitted = false;
@@ -424,39 +431,31 @@ represent(const var_diff_sptr &diff,
bool begin_with_and = false;
// Have we reported a size change already?
bool size_reported = false;
- // Are we reporting a change to an anonymous member?
- bool is_strict_anonymous_data_member_change = false;
- string pretty_representation = o->get_pretty_representation();
- bool show_size_offset_changes = ctxt->get_allowed_category()
- & SIZE_OR_OFFSET_CHANGE_CATEGORY;
-
- if (is_anonymous_data_member(o) && is_anonymous_data_member(n))
+ if (is_strict_anonymous_data_member_change)
{
- is_strict_anonymous_data_member_change = true;
- string tr1 = o->get_pretty_representation();
- string tr2 = n->get_pretty_representation();
- type_base_sptr t1 = o->get_type(), t2 = n->get_type();
- if (tr1 != tr2)
+ const string n_pretty_representation = n->get_pretty_representation();
+ const type_base_sptr o_type = o->get_type(), n_type = n->get_type();
+ if (o_pretty_representation != n_pretty_representation)
{
show_offset_or_size(indent + "anonymous data member at offset",
- offset1, *ctxt, out);
+ o_offset, *ctxt, out);
out << " changed from:\n"
- << indent << " " << tr1 << "\n"
+ << indent << " " << o_pretty_representation << "\n"
<< indent << "to:\n"
- << indent << " " << tr2 << "\n";
+ << indent << " " << n_pretty_representation << "\n";
begin_with_and = true;
emitted = true;
}
- else if (get_type_name(t1) != get_type_name(t2)
- && is_decl(t1) && is_decl(t2)
- && is_decl(t1)->get_is_anonymous()
- && is_decl(t2)->get_is_anonymous())
+ else if (get_type_name(o_type) != get_type_name(n_type)
+ && is_decl(o_type) && is_decl(n_type)
+ && is_decl(o_type)->get_is_anonymous()
+ && is_decl(n_type)->get_is_anonymous())
{
out << indent << "while looking at anonymous data member '"
- << tr1 << "':\n"
+ << o_pretty_representation << "':\n"
<< indent << "the internal name of that anonymous data member"
"changed from:\n"
<< indent << " " << get_type_name(o->get_type()) << "\n"
@@ -472,36 +471,20 @@ represent(const var_diff_sptr &diff,
}
else if (filtering::has_anonymous_data_member_change(diff))
{
+ ABG_ASSERT(o_anon != n_anon);
// So we are looking at a non-anonymous data member change from
// or to anonymous data member.
- if (is_anonymous_data_member(o))
- {
- string repr = "anonymous data member "
- + o->get_pretty_representation()
- + " at offset";
-
- show_offset_or_size(indent + repr, offset1, *ctxt, out);
- out << " became data member '"
- << n->get_pretty_representation()
- << "'\n";
- }
- else
- {
- ABG_ASSERT(is_anonymous_data_member(n));
- string repr = "data member "
- + o->get_pretty_representation()
- + " at offset";
-
- show_offset_or_size(indent + repr, offset1, *ctxt, out);
- out << " became anonymous data member '"
- << n->get_pretty_representation()
- << "'\n";
- }
+ const string n_pretty_representation = n->get_pretty_representation();
+ out << indent << (o_anon ? "anonymous " : "")
+ << "data member " << o_pretty_representation;
+ show_offset_or_size(" at offset", o_offset, *ctxt, out);
+ out << " became " << (n_anon ? "anonymous " : "")
+ << "data member '" << n_pretty_representation << "'\n";
begin_with_and = true;
emitted = true;
}
- else if (diff_sptr d = diff->type_diff())
+ else if (const diff_sptr d = diff->type_diff())
{
if (ctxt->get_reporter()->diff_to_be_reported(d.get()))
{
@@ -511,7 +494,7 @@ represent(const var_diff_sptr &diff,
<< "' changed";
else
out << indent
- << "type of '" << pretty_representation << "' changed";
+ << "type of '" << o_pretty_representation << "' changed";
if (d->currently_reporting())
out << ", as being reported\n";
@@ -529,7 +512,7 @@ represent(const var_diff_sptr &diff,
}
}
- if (!filtering::has_anonymous_data_member_change(diff) && name1 != name2)
+ if (!filtering::has_anonymous_data_member_change(diff) && o_name != n_name)
{
if (filtering::has_harmless_name_change(o, n)
&& !(ctxt->get_allowed_category()
@@ -546,7 +529,7 @@ represent(const var_diff_sptr &diff,
out << indent;
else
out << ", ";
- out << "name of '" << name1 << "' changed to '" << name2 << "'";
+ out << "name of '" << o_name << "' changed to '" << n_name << "'";
report_loc_info(n, *ctxt, out);
emitted = true;
}
@@ -561,7 +544,7 @@ represent(const var_diff_sptr &diff,
begin_with_and = false;
}
else if (!emitted)
- out << indent << "'" << pretty_representation << "' ";
+ out << indent << "'" << o_pretty_representation << "' ";
else
out << ", ";
if (get_data_member_is_laid_out(o))
@@ -572,7 +555,7 @@ represent(const var_diff_sptr &diff,
}
if (show_size_offset_changes)
{
- if (offset1 != offset2)
+ if (o_offset != n_offset)
{
if (begin_with_and)
{
@@ -584,17 +567,17 @@ represent(const var_diff_sptr &diff,
out << indent;
if (is_strict_anonymous_data_member_change)
out << "anonymous data member ";
- out << "'" << pretty_representation << "' ";
+ out << "'" << o_pretty_representation << "' ";
}
else
out << ", ";
- show_numerical_change("offset", offset1, offset2, *ctxt, out);
+ show_numerical_change("offset", o_offset, n_offset, *ctxt, out);
maybe_show_relative_offset_change(diff, *ctxt, out);
emitted = true;
}
- if (!size_reported && size1 != size2)
+ if (!size_reported && o_size != n_size)
{
if (begin_with_and)
{
@@ -606,12 +589,12 @@ represent(const var_diff_sptr &diff,
out << indent;
if (is_strict_anonymous_data_member_change)
out << "anonymous data member ";
- out << "'" << pretty_representation << "' ";
+ out << "'" << o_pretty_representation << "' ";
}
else
out << ", ";
- show_numerical_change("size", size1, size2, *ctxt, out);
+ show_numerical_change("size", o_size, n_size, *ctxt, out);
maybe_show_relative_size_change(diff, *ctxt, out);
emitted = true;
}
@@ -624,7 +607,7 @@ represent(const var_diff_sptr &diff,
begin_with_and = false;
}
else if (!emitted)
- out << indent << "'" << pretty_representation << "' ";
+ out << indent << "'" << o_pretty_representation << "' ";
else
out << ", ";
out << "elf binding changed from " << o->get_binding()
@@ -639,7 +622,7 @@ represent(const var_diff_sptr &diff,
begin_with_and = false;
}
else if (!emitted)
- out << indent << "'" << pretty_representation << "' ";
+ out << indent << "'" << o_pretty_representation << "' ";
else
out << ", ";
out << "visibility changed from " << o->get_visibility()
@@ -656,7 +639,7 @@ represent(const var_diff_sptr &diff,
begin_with_and = false;
}
else if (!emitted)
- out << indent << "'" << pretty_representation << "' ";
+ out << indent << "'" << o_pretty_representation << "' ";
else
out << ", ";
@@ -675,7 +658,7 @@ represent(const var_diff_sptr &diff,
begin_with_and = false;
}
else if (!emitted)
- out << indent << "'" << pretty_representation << "' ";
+ out << indent << "'" << o_pretty_representation << "' ";
else
out << ", ";
@@ -691,7 +674,7 @@ represent(const var_diff_sptr &diff,
;
else if (!emitted)
// We appear to have fallen off the edge of the map.
- out << indent << "'" << pretty_representation
+ out << indent << "'" << o_pretty_representation
<< "' has *some* difference - please report as a bug";
else
// do nothing
--
2.26.0.110.g2183baf09c-goog
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] abidiff: Document and refresh tests.
2020-04-08 12:20 [PATCH 1/2] Improve readability of represent helper function Giuliano Procida
@ 2020-04-08 12:20 ` Giuliano Procida
2020-04-08 12:54 ` Matthias Maennich
2020-04-14 11:24 ` Dodji Seketeli
2020-04-08 12:47 ` [PATCH 1/2] Improve readability of represent helper function Matthias Maennich
1 sibling, 2 replies; 10+ messages in thread
From: Giuliano Procida @ 2020-04-08 12:20 UTC (permalink / raw)
To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich
Following on from giving some test file more descriptive names, this
patch actually documents in brief the purpose of these recently added
tests.
It also improves the coverage of the test-leaf-cxx-members test to
include deletion and insertion report text as well.
* tests/data/test-abidiff-exit/test-leaf-cxx-members-v1.cc:
Comment test. Reorder members of ops to get better coverage.
* tests/data/test-abidiff-exit/test-leaf-fun-type-v1.cc:
Comment test.
* tests/data/test-abidiff-exit/test-leaf-more-v1.cc: Ditto.
* tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc:
Comment test. Update comment on ops2.
* tests/data/test-abidiff-exit/test-leaf-redundant-v1.c:
Comment test.
* tests/data/test-abidiff-exit/test-leaf-stats-v1.cc: Ditto.
* tests/data/test-abidiff-exit/test-leaf-cxx-members-report.txt:
Update locations. Update report to reflect deletion,
insertion and changed members (was previously changed only).:
* tests/data/test-abidiff-exit/test-leaf-peeling-report.txt:
Update locations.
* tests/data/test-abidiff-exit/test-leaf-redundant-report.txt:
Ditto.:
* tests/data/test-abidiff-exit/test-leaf-cxx-members-v0.cc:
Added one line comment referring to -v1 source file.
* tests/data/test-abidiff-exit/test-leaf-fun-type-v0.cc: Ditto.
* tests/data/test-abidiff-exit/test-leaf-more-v0.cc: Ditto.
* tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc: Ditto.
* tests/data/test-abidiff-exit/test-leaf-redundant-v0.c: Ditto.
* tests/data/test-abidiff-exit/test-leaf-stats-v0.cc: Ditto.
* tests/data/test-abidiff-exit/test-leaf-cxx-members-v0.o: Recompiled.
* tests/data/test-abidiff-exit/test-leaf-cxx-members-v1.o: Ditto.
* tests/data/test-abidiff-exit/test-leaf-fun-type-v0.o: Ditto.
* tests/data/test-abidiff-exit/test-leaf-fun-type-v1.o: Ditto.
* tests/data/test-abidiff-exit/test-leaf-more-v0.o: Ditto.
* tests/data/test-abidiff-exit/test-leaf-more-v1.o: Ditto.
* tests/data/test-abidiff-exit/test-leaf-peeling-v0.o: Ditto.
* tests/data/test-abidiff-exit/test-leaf-peeling-v1.o: Ditto.
* tests/data/test-abidiff-exit/test-leaf-redundant-v0.o: Ditto.
* tests/data/test-abidiff-exit/test-leaf-redundant-v1.o: Ditto.
* tests/data/test-abidiff-exit/test-leaf-stats-v0.o: Ditto.
* tests/data/test-abidiff-exit/test-leaf-stats-v1.o: Ditto.
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
.../test-leaf-cxx-members-report.txt | 17 +++++++-------
.../test-leaf-cxx-members-v0.cc | 6 ++---
.../test-leaf-cxx-members-v0.o | Bin 7328 -> 7328 bytes
.../test-leaf-cxx-members-v1.cc | 13 +++++++----
.../test-leaf-cxx-members-v1.o | Bin 7360 -> 7368 bytes
.../test-leaf-fun-type-v0.cc | 1 +
.../test-abidiff-exit/test-leaf-fun-type-v0.o | Bin 2840 -> 2864 bytes
.../test-leaf-fun-type-v1.cc | 4 +++-
.../test-abidiff-exit/test-leaf-fun-type-v1.o | Bin 2952 -> 2976 bytes
.../test-abidiff-exit/test-leaf-more-v0.cc | 1 +
.../test-abidiff-exit/test-leaf-more-v0.o | Bin 3792 -> 3800 bytes
.../test-abidiff-exit/test-leaf-more-v1.cc | 8 ++++---
.../test-abidiff-exit/test-leaf-more-v1.o | Bin 3808 -> 3832 bytes
.../test-leaf-peeling-report.txt | 12 +++++-----
.../test-abidiff-exit/test-leaf-peeling-v0.cc | 1 +
.../test-abidiff-exit/test-leaf-peeling-v0.o | Bin 4528 -> 4528 bytes
.../test-abidiff-exit/test-leaf-peeling-v1.cc | 22 ++++++++++--------
.../test-abidiff-exit/test-leaf-peeling-v1.o | Bin 4600 -> 4600 bytes
.../test-leaf-redundant-report.txt | 10 ++++----
.../test-leaf-redundant-v0.c | 1 +
.../test-leaf-redundant-v0.o | Bin 3320 -> 3352 bytes
.../test-leaf-redundant-v1.c | 10 ++++----
.../test-leaf-redundant-v1.o | Bin 3384 -> 3416 bytes
.../test-abidiff-exit/test-leaf-stats-v0.cc | 1 +
.../test-abidiff-exit/test-leaf-stats-v0.o | Bin 2784 -> 2800 bytes
.../test-abidiff-exit/test-leaf-stats-v1.cc | 6 +++--
.../test-abidiff-exit/test-leaf-stats-v1.o | Bin 2824 -> 2840 bytes
27 files changed, 65 insertions(+), 48 deletions(-)
diff --git a/tests/data/test-abidiff-exit/test-leaf-cxx-members-report.txt b/tests/data/test-abidiff-exit/test-leaf-cxx-members-report.txt
index eae74a3b..27fdc45c 100644
--- a/tests/data/test-abidiff-exit/test-leaf-cxx-members-report.txt
+++ b/tests/data/test-abidiff-exit/test-leaf-cxx-members-report.txt
@@ -15,28 +15,27 @@ Removed/Changed/Added variables summary: 0 Removed, 0 Changed, 0 Added variable
1 function with some sub-type change:
- [C] 'method virtual int ops::changed_fn()' at test-leaf-cxx-members-v1.cc:6:1 has some sub-type changes:
+ [C] 'method virtual int ops::changed_fn()' at test-leaf-cxx-members-v1.cc:5:1 has some sub-type changes:
return type changed:
type name changed from 'int' to 'long int'
type size changed from 32 to 64 (in bits)
-'struct ops at test-leaf-cxx-members-v0.cc:1:1' changed:
+'struct ops at test-leaf-cxx-members-v0.cc:2:1' changed:
type size changed from 128 to 192 (in bits)
1 member function deletion:
- 'method virtual int ops::deleted_fn()' at test-leaf-cxx-members-v0.cc:3:1, virtual at voffset 0/1 {_ZN3ops10deleted_fnEv}
+ 'method virtual int ops::deleted_fn()' at test-leaf-cxx-members-v0.cc:6:1, virtual at voffset 1/1 {_ZN3ops10deleted_fnEv}
1 member function insertion:
- 'method virtual long int ops::added_fn()' at test-leaf-cxx-members-v1.cc:3:1, virtual at voffset 0/1 {_ZN3ops8added_fnEv}
+ 'method virtual long int ops::added_fn()' at test-leaf-cxx-members-v1.cc:11:1, virtual at voffset 1/1 {_ZN3ops8added_fnEv}
there are member function changes:
'method virtual int ops::changed_fn()' has some changes:
return type changed:
type name changed from 'int' to 'long int'
type size changed from 32 to 64 (in bits)
+ 1 data member deletion:
+ 'int ops::deleted_var', at offset 96 (in bits) at test-leaf-cxx-members-v0.cc:5:1
+ 1 data member insertion:
+ 'long int ops::added_var', at offset 128 (in bits) at test-leaf-cxx-members-v1.cc:10:1
there are data member changes:
- type 'int' of 'ops::deleted_var' changed:
- type name changed from 'int' to 'long int'
- type size changed from 32 to 64 (in bits)
- and name of 'ops::deleted_var' changed to 'ops::added_var' at test-leaf-cxx-members-v1.cc:2:1
type 'int' of 'ops::changed_var' changed:
type name changed from 'int' to 'long int'
type size changed from 32 to 64 (in bits)
- and offset changed from 96 to 128 (in bits) (by +32 bits)
diff --git a/tests/data/test-abidiff-exit/test-leaf-cxx-members-v0.cc b/tests/data/test-abidiff-exit/test-leaf-cxx-members-v0.cc
index 89612562..e145375e 100644
--- a/tests/data/test-abidiff-exit/test-leaf-cxx-members-v0.cc
+++ b/tests/data/test-abidiff-exit/test-leaf-cxx-members-v0.cc
@@ -1,9 +1,9 @@
+// See comments in -v1.c file.
struct ops {
- int deleted_var;
- virtual int deleted_fn() { return 0; }
-
int changed_var;
virtual int changed_fn() { return 0; }
+ int deleted_var;
+ virtual int deleted_fn() { return 0; }
};
void reg(ops& x) {
diff --git a/tests/data/test-abidiff-exit/test-leaf-cxx-members-v0.o b/tests/data/test-abidiff-exit/test-leaf-cxx-members-v0.o
index 8202ecb5dd7cb2bbe79a0a2f3ff42a103f56387c..cf5c9eb87a1dd22db9e7f0532de1530362946c69 100644
GIT binary patch
delta 302
zcmZ2rxxjKmEF&Y+<T%C_K4x|$Mg|5BP6i-gWSRVd@dhK)=95fr%#5Ow1=tK3g(o|)
zrB43K)*#{T7pmZ_t!-$cV5w)MXQ*IgU}$M*Xka;cKbzg;T6S+iOHDMH%{SRiSSBm;
zh)lL%=ih9=<G{#xbaElDxMU#6HXsm%Pz+o^S^$XqCokl6mplL!wTIF$+1$yDeDR#C
zprRWlC-RC5?nP50KY1dbxa1V5=wme5Et3!OiBF!u$07I~O{Q}5M?QCE*5r)Dy!6So
QLgJIH*m*Z63fXf30L}R}<p2Nx
delta 320
zcmZ2rxxjKmEF&Z1<T%C_J|=c0Mg|5BP6i-gWS;zi@dhK~=95fr%#0$F1=tLMs+`zT
zCEWc&6`ZxT4NVj*^^Eik6^slFEe#C~j3(Q&+f821)-bu2-CNLH6HRvWO?DHO$;vz;
zlg)(qHyiLcFfzJMF60%LTn@4g2t*+i0~e4M0OF^U7xKDGiUA!20`?FJE_-+~BVW8B
z15|bcnk*Mod@q`~*5rwN;*#f~qL0yJXHP!JCq8)sABW&~G?`bMKk~UVv!<lxq?Sy!
Q6%wCpA;h;iNyv^10JSVM`v3p{
diff --git a/tests/data/test-abidiff-exit/test-leaf-cxx-members-v1.cc b/tests/data/test-abidiff-exit/test-leaf-cxx-members-v1.cc
index 0437584e..892775fe 100644
--- a/tests/data/test-abidiff-exit/test-leaf-cxx-members-v1.cc
+++ b/tests/data/test-abidiff-exit/test-leaf-cxx-members-v1.cc
@@ -1,11 +1,14 @@
+// Exercises the reporting of struct differences in --leaf-changes-only mode.
+// The resulting report should have no excess blank lines.
struct ops {
- // TODO: type, name and size are differnent from deleted_var, but this is
- // still reported as a change rather than a deletion and an addition.
+ long changed_var; // was int
+ virtual long changed_fn() { return 0; } // was int
+ // Note that in order for this to be treated as an addition, as opposed to a
+ // change, we need to make sure it's at a different offset from anything in
+ // the old version of ops; having type, name and size different but the same
+ // offset won't work.
long added_var;
virtual long added_fn() { return 0; }
-
- long changed_var;
- virtual long changed_fn() { return 0; }
};
void reg(ops& x) {
diff --git a/tests/data/test-abidiff-exit/test-leaf-cxx-members-v1.o b/tests/data/test-abidiff-exit/test-leaf-cxx-members-v1.o
index 617ffd21551b158227af04c6e601ebc5a12e6b9b..9b74afbffc7868e4549e6cc65fa456b5cd462976 100644
GIT binary patch
delta 662
zcmZ9IOK1~O6o%)_IGIV>%uI$R<4r0}#YaM<$%LW~N{1?#Lah`*&`o0@HVTpiALv5t
z1B;K1h99Z}UDSo((lVPOh~ma9m2S*ZT<A^|#Nx`Cbc8Yo?tjkt{&V5n`_AfEPjd8N
zJ%5}?z#QFEcFEJkadMRNf<$c5ekImJTC9iqP~hvQl!)K_kG(-bt1I5!<ZX@=)vx&W
zM9Q~FmGWNV+tZvX`;~Fcr-J*LmjznI7ja@TB_3E3^XGY<BN8<(%6k*N5Mm_mTM`cj
zmb}9nc+H#sxW0o4aY8FnmuG9WOSS4;1BWCgUu`z(*(;43n3Bf!+-<h5bComavL_1#
zb2Mv>lt;{Lxnx?VS+ejWG6qMQOj`Mn<1gd>mW~N@0%qSY+|pGvqyd!rBkt+`QKAo6
zj2Gz<*5eiSetUP$4zmAuns-`AC{=SKpszx1E$H8b_+dO(4E8vnUqkLVJ}Cwk;&S3o
z$aw)$i~Jo9Wp##@(N?RT{#1*!><(*9qMI-i20g^MHiS-M#e0&5vpRF{>j{B2;3U7%
z6c$rY=_8miPv7BqS_0esmM)^}X$cOSr<Y*q3aw!_eS|K<N!zr5uBV$|8JqtrfhdJm
iCWodWV=Z%(I_PF>dImOV(-tla+R>c8z4{D>+0Z{P`GJW5
delta 651
zcmZ9I&1(};6vgMhIGIVC&LpYL<fF)pRGScJJ29wH$xyVW2$n7sf(zqdY!oCZZ3!-l
zl_H2M6At)+T{!*)4Z($iSiwbI3Ek+zg;}|cT}WLBzKJQ69C+v6_dEAJcyBwqm8~b}
zK{I)dh_FBp6(-M<Ajm#}i#*t)V~W^Cfq^daBE`xVeO`C7ucN(&HlFgcBliUIscy#_
zvtc))0V&A~Ys?F(>~`9cOR4pZ$sT%+wm6$VERL;<;|0bz7XyNi@rkZ9@%Xx!>;>|H
zfZj3F9S$8}F|2sYk^J0M{d_8AX7ubtdcxGxQ)bpQr_v}&IsN8syHdVYD%R{Jp?GcX
zM0IibLT08W*rgJu3l$uaa{jy3N?E^CSrP767x7ZM96XimyQ}qAI_n_|*}+{*`-c&h
z!mTU*456dQs1GIRE8Y&}qY?fMuB`t>a6d7?*Ly^jVpQy<n|<jt8j2SE+@qiR(p`My
zX|qQ=ed$U3QZ)JwpE}7@!Ws2C?cjx4<o-vUq@7kWP$gPMBcRbEG=pyjPwzi{riMc3
zNb5-`=%GzGBR}a99*3XNHVkcze!$_LfD!pX1+=**;6&Hx6^vM(HsHv^SdER*4LC82
lR?+7E1xDPW->~Br+mrA*o}(?aIjX}LwWtG|yAEe`{2!|-iuV8j
diff --git a/tests/data/test-abidiff-exit/test-leaf-fun-type-v0.cc b/tests/data/test-abidiff-exit/test-leaf-fun-type-v0.cc
index b75dd84c..db1f7917 100644
--- a/tests/data/test-abidiff-exit/test-leaf-fun-type-v0.cc
+++ b/tests/data/test-abidiff-exit/test-leaf-fun-type-v0.cc
@@ -1,3 +1,4 @@
+// See comments in -v1.c file.
struct ops {
void (*munge)(int x);
};
diff --git a/tests/data/test-abidiff-exit/test-leaf-fun-type-v0.o b/tests/data/test-abidiff-exit/test-leaf-fun-type-v0.o
index 211a33884f30755929210fee5d328e5b7e6c24e1..f03d386fc1156eac6e48a6d7b46f9b0153f4197e 100644
GIT binary patch
delta 312
zcmZ9FF-yZx9K_%M<)!95a1cs^LbbYR#WYEmHVBQlw40l=Qc^ltX*8BPI@`f;OD91G
zXG<0{xjMMH`w^u50%Ek{;Bdzc_q!Vx9t$^H)Q`)Di`-~2#<Wa~w0N}5bGi5^AHTj~
zniHDFY&_8FhSj+^wW7;@U=5r~yZzl}`Wln`CuL^hor;?4c=)6S_Jvurqh6my!Fgnz
z1jn6!1L*Sdl2<Nym1@N;RUOxJGd8*B1ELiyi*@-2U{?fmhC5+W52<L<7^Iu>`uDkq
zSeU5mEovlJy3J{b$-*;r5gJW8Kx)+Bs62$+ksmZ?ZsExqjSxzo5==6G!c6*@Dg$*@
Urzt|^%RMn~yGT`yUNBReUwJ`L&Hw-a
delta 357
zcmdlWHbZQJ24loT%`jd@4i5$f24)~;U}T!uqs7F^HZehD;+f0r&R}u%$;%k!7>y>M
zWt319V`Xe+VrF1uboUEYaMso~G*PhBGtx6uFfuT-G&C?an9Rmxr&E$zT%wzknwVy!
zTV|k_oXij(WoT5Cnx0u)l3Em>Ur-$6Z44wC^h<IJCRZ?N3Yux68MAp4QyC+p&}2;(
zMWD^WEUA)VAol@*D1>6*0@9p7TsQe5i@W3msAw9RZ1ZG8R&mDk$)2qKjAoMqS;aSR
zV3lBEOqhI<eLrKs<W3HEM)%1VIm{UwCUbJyb3S8)=$|$@kkg#8W^yN|J=0>Q$sd`-
yCO_cRU|cbolgoZG2dBv77%m;g2a`Lw>^TisAX+phU*t09+`tOqEuMUlO&$QU?N00f
diff --git a/tests/data/test-abidiff-exit/test-leaf-fun-type-v1.cc b/tests/data/test-abidiff-exit/test-leaf-fun-type-v1.cc
index f308a343..ccf79dbb 100644
--- a/tests/data/test-abidiff-exit/test-leaf-fun-type-v1.cc
+++ b/tests/data/test-abidiff-exit/test-leaf-fun-type-v1.cc
@@ -1,5 +1,7 @@
+// The change to ops::munge is local to ops and should result in ops being
+// reported as a changed type in --leaf-changes-only mode.
struct ops {
- char (*munge)(long x, bool gunk);
+ char (*munge)(long x, bool gunk); // everything but the name changed
};
void register_ops(const ops&) {
diff --git a/tests/data/test-abidiff-exit/test-leaf-fun-type-v1.o b/tests/data/test-abidiff-exit/test-leaf-fun-type-v1.o
index 9db3ed5e8f997d9c8455f8361449ce516edc840a..2a23514d593c02f79d432e6ee7c3d5a8aa0b1616 100644
GIT binary patch
delta 448
zcmZ8c%Sr-a6#ma#axO$hhBHAjf>>gsjudEwA_ltBRc%`6DB}f8Z5)F(LDZr}i{!k3
z?OG;oS_Hm9yP#DmJwb2K)JO{te0*@e%i-L5hu-^bm<~e6%QTlKgwQq!FoU}c<ysn)
zJ%g_|EV~$*CQ+P#k5_B`wrX8;4g3H}l32K=XaaEBZ`Tb{ZRjScbUH2UQo)pE^enYy
z=(YchdagyPRbm?TX3sLrQm5M^xx$f>4TrT|B^ilFG$kI>lA0Dv;wF^}l3X^suWTP0
zm8RZy462q4Em+5|lnJnb8OFb|a0@Rf39pzaoZyJ@%6q@x*|Y8t5p3pd$T(w4_{4CS
zXWg=Uu9^!j5zkn!T$pcf7E~WTuti9Xzu71SL-cbKc*hx+gCefdYk0s1a42}t;&*r#
w<zz>H$_pQWXPohA_(Z>uhAHYo8p|Swk3s?@oC#^Z!8i%c=@J8wKwaGW0doRov;Y7A
delta 418
zcmZ1=-XT6ggYm>f%`jd@4i5$f24)~;U}T!u<H*FyHZehD;^V9A&R}u%$@>}Q)Qxn@
z4E2(eL8=&7<rz4cz$_pk#>&{t#LU3Rm|L2co;o>^NsQUR*kE!MlbvW%etr%^NosM4
zZcb`q8q|czVoYil$r*`73`MEwnZ+fkMe+Fs#S9GY&dyc}8ZN0xnTdG{mU>2dhPsAk
znh=(Pk%6J5A<)>(_n3+p8HFYXvM4edO|E9~m1KjuLl8=H0clPkPMrLZMVzsE@=um{
z$>~tJY&11RlM7kJC1aqXwP><)CNE?aXKa|flhvQmY%(LO_+|w*2`0vl$(9`Z84pbU
z$>GlEKG~7eoaqDW<SI@(&i{-M16EEx$Z5{FV)9Q;d#3YDlNU0IO%CAFV7xH7lFNQ_
l1*gd5Gh8~19Fu=?*>gs)K(uI1cH}naRDk+r@nlDKc>vtxSepO<
diff --git a/tests/data/test-abidiff-exit/test-leaf-more-v0.cc b/tests/data/test-abidiff-exit/test-leaf-more-v0.cc
index bb6505c8..d67978b1 100644
--- a/tests/data/test-abidiff-exit/test-leaf-more-v0.cc
+++ b/tests/data/test-abidiff-exit/test-leaf-more-v0.cc
@@ -1,3 +1,4 @@
+// See comments in -v1.c file.
struct changed {
int foo = 0;
};
diff --git a/tests/data/test-abidiff-exit/test-leaf-more-v0.o b/tests/data/test-abidiff-exit/test-leaf-more-v0.o
index a8734481238bda41217440b305307ea7c5464782..b3a3084e0e6061e2ac4f7b3ffae6cf81cb519f55 100644
GIT binary patch
delta 621
zcmZ8fzi-n(6u#$+?X#OyH7IdXXyYJMY@)<*1VrvYDg`MKMHGfgAS6miT^gz62aOPH
z6)Y@BSWo=}s!ox*luk^jTL&bvATg2=w(<uc&W^kEhVOm%z3<(<`!SmPIrqtgv~O+C
z3amj0p&Q*U0V)TakXTG?cLx5q`IHz43Jp~bVnBfTe=flcN|4oNuK8RM{7s^SsRZ7F
z9H{sR{>o3dLP;JU3iD#&W%rebJt1AD3W(~m2#>|fc0{h*BD@zU0T_7OgTj{A+z9=J
z*QH{sMQ~rp<1;CVPbD2M(wy4bY<6CF>!jVmEKTAby_v4xUo~p^ymP~Fi{+wYluM55
zI3*YF(c258UaKt{<{j^OyV;o-3XW@?vmX7Up8-;MDC(K3ymgH?9}5n}$636BW1e-n
za)CgY31BK>cTtmcSVv3BMF(D%-r|X5!Wd6^?>)vc$;hkn7+l#XnrHBs>9{1DvmYbn
zK@`uNKM1~)i?D=;vJI;^=D3R*)8POuW@p|+_V4G)lGtZ9Y~dbv+W3X7z+$k~k)bpM
z7-4(rEBrz$UWYa8$1D6<hw-X16eg=w@RVyjpQs9Q3^g@{I|&mQ4ii-{@i)gEw5F>{
VLz)zP!2Wba>4l$xkbUf{+5Z9OcTxZV
delta 586
zcmZ8eO>0v@6rDToW8Sk#+Sk&?7ioNTlf=*@6Ez`Fj3BgKSqc_)r6EllL0U_L80eyf
zE~K0KJU>9tKVVuA+$iqcsT=9iozz|3by0D~zHD+A_ndp~xpRk^qsecRFS9ff<~B>@
z%rnM}mGyN-TxiaiX<CohI?U3XEK7^7p$iq^`*isG5)H$q%ieG%R1y!{%+)=Lp+ZyS
z;y(QmOG+?U28%qMlGD#hg=SNGm^H>+{-3Fo!ah&oBhP!C)@En=NvpBa_r5a5MYG*l
zTWu}ljMpyC-B}85=kxM<P%D%RGANg1P0CUkZ;YFGXHVd+QNd4Re5{%qa4>dgn+N2N
zs)e4qQ$Ngz1o&<x{dClIBDIe<Kz7ofRq7}sHWYRH_z2RTB8hu;S09Y8cFxytqQg4}
zc0r+jpFBK~<W~q+#VjpiLlqgqo+!`^K8PYcz_BR$zoMzv1Ji#Yog!_a;UqC49BeyT
z|7qko8Stj?*;$~<q31s2^aUG3pXdlOHm6o}V^wm{i`Ct?(Jx%XsnUKx#_Rmdz>YbA
g7x64H^x}02aH8^a$VA<}WuLFvMK@8UHhPK7Ke8ofP5=M^
diff --git a/tests/data/test-abidiff-exit/test-leaf-more-v1.cc b/tests/data/test-abidiff-exit/test-leaf-more-v1.cc
index 5df5df9d..7f5b65d3 100644
--- a/tests/data/test-abidiff-exit/test-leaf-more-v1.cc
+++ b/tests/data/test-abidiff-exit/test-leaf-more-v1.cc
@@ -1,12 +1,14 @@
+// Ensure that local changes to functions and variables (including their removal
+// or addition) are reported in --leaf-changes-only mode.
struct changed {
- long foo = 0;
+ long foo = 0; // was int
};
-long directly_changed_var = 0;
+long directly_changed_var = 0; // was int
changed * indirectly_changed_var;
long added_var = 0;
-long directly_changed_fun() {
+long directly_changed_fun() { // was int
return 0;
}
void indirectly_changed_fun(changed * x) {
diff --git a/tests/data/test-abidiff-exit/test-leaf-more-v1.o b/tests/data/test-abidiff-exit/test-leaf-more-v1.o
index 324180ca92b91045edb4c9d3b9fef1fd314a09ba..4ac71798c2735b828957689acf622d267f60f67b 100644
GIT binary patch
delta 685
zcmZ8fOK1~O6utLNX68+PG=+8&rEQ`Mlg3FhK}aW{0V8dos0~!G?IMiH#1?8IX%Z;9
z3Ej9%xGUFPxohpJ8>LJ4E_CN->q5HHjrb-REA%byIrp4%;oZmkGWTn4n8c+~YHwZQ
z6#$T$?KYrq%mI8{78;BTs)`%XBv*0IMCE}Og8x?)Zd`cU=;sVhk%9>fQxX><;Tvw4
zjkZA*2pFKpc&=^<DnXj6Y+g2=wL1<yXVJXmN7=tB<A%JLmGMBHzCCFf1H`Vg*EMz>
zyE)DqeKS+910AyK*BUz=r{3M&uhqBh)>EfZ>)RcuZCZAtA)02d)u*a5PZ23bKiE|w
zsSsUx@F-qRr_Eb&E0fQd@qEs-OfzTEUFptESSgo_@#H<{$&THc1Ps$kU5dKf0G#}h
z-XccnSk_~+qK}BR_}GC9H0Y~v3qqX+^gMuo$Ep-jbnH<|NqA4Os`MBDy;Bxr-@Q=8
zL%%@H<%FuvJ_21)mvKb4s>iZkWX;2W(IpzGS=^#CHOoE&ovRr-;+&Fviroh)@=WZt
zH*t1p@`wgJi!bSz>o`Ln_(LpGLe)=lT7}^!+7G;Epiwcnj5Qhr3z(zRU`cx;?(QCo
zFjT;AWQ9sh^HIV-OT$o7{EgF42`ltl)K_GMOWK+;891OpxS$Qk>j50nz`yVp@3?(T
delta 640
zcmZ9J&1(}u7{=$F-R#aLo5i$EH@gx|NFWI%n@+6Ocrbo|U&W6U3wkP2T6!py+A4wv
z@#ImL=j2I2FG5fD?#F*%ZuKBkPads^c+=TrRLH>0^YFaCdEbHgRop8+sZ%L#Y+fgE
zfiY&RtgkcT1J9T#QYvGEgHl!kI7X5rYnWP@`^x`MEz$f}v*T}h@?cS!T-51s&aEXd
zk5`$kl#zoQ^xa-i)?hXC_@PiX@5*RtDXFqlkvV+e)Dhw_5AlXq4s0!NZZ+4IZ>?w+
zdwdcnjDzrvO5yyKh0@unDLGT>MD0kH+S9Tl<#Zb_jgvTIT*6Q8;fs;a%{E4e9nXAC
zG=cXiKNwR3bLve(Pyee8?5pY#m5wpeHO4a9T}IjRX#*XrOcIOM9qd|ldIYllaz!(F
zt!br>+6CPvfOLFZv+Kb{P0WpuA!6I78N9Mv)Wb)+#fyZrh{9D3J{!4EBg~74KH;J8
z=?9*QR&ZM*`y(jBXK|H|?MP>l(>uhOxBRQXynBvrA~wgd?N+IPzT0(P=npxHAUjLD
v=w-Y7w}I2?Njx`AY-g*)(9d@9!4)WY$LSt=Ue`Hg4JSSrw)3hg?|ZdBB6Vuj
diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-report.txt b/tests/data/test-abidiff-exit/test-leaf-peeling-report.txt
index 7ab4dd6e..48f5df8e 100644
--- a/tests/data/test-abidiff-exit/test-leaf-peeling-report.txt
+++ b/tests/data/test-abidiff-exit/test-leaf-peeling-report.txt
@@ -3,38 +3,38 @@ Changed leaf types summary: 6 leaf types changed
Removed/Changed/Added functions summary: 0 Removed, 0 Changed, 0 Added function
Removed/Changed/Added variables summary: 0 Removed, 0 Changed, 0 Added variable
-'struct foo at test-leaf-peeling-v0.cc:1:1' changed:
+'struct foo at test-leaf-peeling-v0.cc:2:1' changed:
type size changed from 32 to 64 (in bits)
there are data member changes:
type 'int' of 'foo::z' changed:
type name changed from 'int' to 'long int'
type size changed from 32 to 64 (in bits)
-'struct ops1 at test-leaf-peeling-v0.cc:5:1' changed:
+'struct ops1 at test-leaf-peeling-v0.cc:6:1' changed:
type size hasn't changed
there are data member changes:
type 'int*' of 'ops1::x' changed:
pointer type changed from: 'int*' to: 'int**'
-'struct ops2 at test-leaf-peeling-v0.cc:9:1' changed:
+'struct ops2 at test-leaf-peeling-v0.cc:10:1' changed:
type size changed from 320 to 640 (in bits)
there are data member changes:
'foo ops2::y[10]' size changed from 320 to 640 (in bits) (by +320 bits)
-'struct ops3 at test-leaf-peeling-v0.cc:13:1' changed:
+'struct ops3 at test-leaf-peeling-v0.cc:14:1' changed:
type size hasn't changed
there are data member changes:
type 'void (int&)*' of 'ops3::spong' changed:
pointer type changed from: 'void (int&)*' to: 'void (int&&)*'
-'struct ops4 at test-leaf-peeling-v0.cc:17:1' changed:
+'struct ops4 at test-leaf-peeling-v0.cc:18:1' changed:
type size hasn't changed
there are data member changes:
type 'int*' of 'ops4::x' changed:
entity changed from 'int*' to 'int&'
type size hasn't changed
-'struct ops5 at test-leaf-peeling-v0.cc:21:1' changed:
+'struct ops5 at test-leaf-peeling-v0.cc:22:1' changed:
type size hasn't changed
there are data member changes:
type 'int*' of 'ops5::x' changed:
diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc
index 745d44f5..2a25b877 100644
--- a/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc
+++ b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc
@@ -1,3 +1,4 @@
+// See comments in -v1.c file.
struct foo {
int z;
};
diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v0.o b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.o
index e6ed6218704e957cbac929f82d0a571fd181103f..cf5fc57b45f185b13b0d61ec9150f25e5c952b69 100644
GIT binary patch
delta 559
zcmZ8dy-Pw-6u;-ar``7+7H0N*wrHkU)YB&oy#^tyglOpxh)A+<un!H&XsDr<Ad~~q
z64BNYNCu735RK8$(9qP-+|U$6_v)SA8Sd|#-v^v~nz3fAey2-xmJlNE5s(`X!4WTA
z<>M}aE|qtLc-saKZ<C<WBZhK~fZq6v`?~2p8#S&$52hhL)Jwm(f#m}t-%e8Krvy5J
zfi;L3VGo~GnP++R@CPE;f;8Sj2!FxEPlltCiG5NC7a7h-q1LHX02sh;If_vl!jzKm
zRf=n+>UOcRw6R$o$1*jstOOJ`*!ZTL;eld+gXc;%LGroWWF$FVER>elBaW4^>@nLJ
z?sSn%+IDO^?X)gEQ?l)6+wt;up#4cz^c;dN{u=#i-rNyv*`@B|mTH&@!Ctu3L%d?@
zgkWD>>M_1D^+d3S*oz;X?^8k0Qm}%yW=J0d=QPXA3Tx42tz&~(Wqi~On8$Z575)*<
WXP5Jd;Gk}?>o4dA+_Xx1-un+qU{qHC
delta 566
zcmZ8d&r1SP5Pq}k%I?YrMz*dIteIh2>JuR>3}P#xAn4E`qM~4nJ!GX(*}ZEdL&u2z
z1HqtU1VPlHh%VJX(6tVog6KWlF8g5kX1*UV^BS>6?D!%8@+2WdJRl&{uR=*lUgP6F
z0l&gKLi{a*#M=ZY@*zX9MnJ9q#5GO6WTVP8Xu~YTdxP>jH?VxWn{Op$2+0KggifqM
zOfw`tD<aPle1j<7gMl4F7{3`#FdPsJ+-0~Z7|_++6KVhv{1l_U&8;d$Q#LT^>%%2q
zDqNbGGp$sL=FLJnm!@VeOAC}{b2uW6;*swpU$NJn>W*DmX3;z$CCeH%6XW)(vr_(x
z4AP-~Pq=v|jfu3}+K-T+i~LrFRqHB-ArJq7L8WB4B-!w&H@L0nM#^PRJ?b5vGqvcl
zj~?{_pP2gWvO4=b{{HuI5Y!ATAXRlC4mhQzjd|B%d#o>5XVz)FR&}uORn0_-uH)9@
U)Bt+|Y3Rn;fDT_xCs6YL0xSAeyZ`_I
diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc
index 439f7c0b..ea0f835a 100644
--- a/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc
+++ b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc
@@ -1,33 +1,35 @@
+// Ensure that different pointer / reference types are considered genuinely
+// different in --leaf-changes-only mode.
struct foo {
- long z;
+ long z; // was int
};
struct ops1 {
- int ** x;
+ int ** x; // was *
};
struct ops2 {
- // A change to foo's size is currently considered local here. Arguably this
- // should be considered non-local as the change to foo is being reported
- // independently. If this happens, the test case will need to be updated (to
- // remove the reporting of an ops5 diff).
+ // A change to foo's size (impacting y's size) is currently considered local
+ // here. Arguably this should be considered non-local as the change to foo is
+ // also being reported independently. If this happens, the test case will
+ // need to be updated (to remove the reporting of the ops2 diff).
foo y[10];
};
struct ops3 {
- void (*spong)(int && wibble);
+ void (*spong)(int && wibble); // was &
};
struct ops4 {
- int & x;
+ int & x; // was *
};
struct ops5 {
- int *** x;
+ int *** x; // was *
};
// TODO: This *should* be considered a local change, but currently is not.
-int var6[5][2];
+int var6[5][2]; // was [2][5]
void register_ops1(ops1*) { }
void register_ops2(ops2*) { }
diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v1.o b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.o
index 81fcfe0a5a5abecf2d104e71859a04ab529d0b04..c59573e19ddfd8d7b170151a743445ca15be1574 100644
GIT binary patch
delta 587
zcmZ9JF-QVY7{}jtp7ri7Ls2;&HJq7(3F?UiHxxwV5rkW7Em^o!XoLkdI5(&tG_*uS
z&=5ozG}h46)>czNQ(HrW5Jd0kYq<ya-tYhay*u9HdWl}5b!+%&i4fx3C!n_PqsA~@
z7qLNrw%_80K|3Pu5eT9I$v~Js@~rcRKm=zXIc3ssv4g)5(*zMRN^5-f3#2ec!!Xd}
zcm!$F@-v<_pRi&C#RMsADJFgZgJho~3WLc$EpSToX_wPPx1}@z;uu!r=%}ect+G+A
z?^J55Tif*_uBg*02OB@t)0kUcw&pV#yJ$JtLe{nldE2q=yn|QjEFSw4At&uI-6wy^
zXOHmni4hts{mW{cA7qYt`g5GqTy`Scvd32OOf%W0Y%e@^6rZ`R%l6J=Ed<?U4cY#9
z>;c-k32V5hyUGi}j-F*Hx#dt^LHs9i^d`RQIrb~<wZ|4QZsgdPwC^6fgjH^(Bq3!$
M|6jiB-WcxCAJ3Omv;Y7A
delta 571
zcmZ9JF-XHe6o&7miChv|thL5-(3G}f71|~gw1c9EEiO(Df`cwqIw`G)Iyg8BrhT{;
z6r4pWIymT{OE*U+MMoD0hk}cDNp#4A-231E-5r0pZnB$nu63X#LP+R<fYi8;>k8cv
z;~s&Y#%n@XX;X~*1R|(IIvk~seAf6!pbw`ZJszjuVh2AkpbAAugx2_~2uXp_7<hOH
zk0E0WD~!)-NKjF|Vu2Jol7Sz<AnB@piQ$Pfhzk;fgzM~bWwssZ2w((bazfeKu3H2P
z@+h|G7&>woXB7i%Tv2B5TyBMHmCb5>r&3$v8w+@<q@t4<vzWEBmTBj$qGjdnc1J0N
ztaXuqSJZ3nEfo>NrZYybWy&Z%Yg?z>9?O7KiI*p2UA1ZWJOIlc4U+W<`?Xa;-m
z-Jbw`LQ6AX6&JO#WCAv|EL-*?9{OFpi-_LA7cIw1zIz#<6PVC*Y|3}v0(2g$oVu^a
RZ;DD@ioct`+E;qH=ND_rP|*MY
diff --git a/tests/data/test-abidiff-exit/test-leaf-redundant-report.txt b/tests/data/test-abidiff-exit/test-leaf-redundant-report.txt
index 34b7a848..581fd28e 100644
--- a/tests/data/test-abidiff-exit/test-leaf-redundant-report.txt
+++ b/tests/data/test-abidiff-exit/test-leaf-redundant-report.txt
@@ -5,23 +5,23 @@ Removed/Changed/Added variables summary: 0 Removed, 0 Changed, 0 Added variable
4 functions with some sub-type change:
- [C] 'function void fn1(sto1)' at test-leaf3-v1.c:10:1 has some sub-type changes:
+ [C] 'function void fn1(sto1)' at test-leaf-redundant-v1.c:12:1 has some sub-type changes:
parameter 1 of type 'struct sto1' changed:
type name changed from 'sto1' to 'stn1'
type size hasn't changed
- [C] 'function void fn2(sto2)' at test-leaf3-v1.c:13:1 has some sub-type changes:
+ [C] 'function void fn2(sto2)' at test-leaf-redundant-v1.c:15:1 has some sub-type changes:
parameter 1 of type 'struct sto2' changed:
type name changed from 'sto2' to 'stn2'
type size changed from 64 to 128 (in bits)
1 data member insertion:
- 'double stn2::y', at offset 64 (in bits) at test-leaf3-v1.c:7:1
+ 'double stn2::y', at offset 64 (in bits) at test-leaf-redundant-v1.c:9:1
- [C] 'function void fn3(sto1*)' at test-leaf3-v1.c:16:1 has some sub-type changes:
+ [C] 'function void fn3(sto1*)' at test-leaf-redundant-v1.c:18:1 has some sub-type changes:
parameter 1 of type 'sto1*' changed:
pointer type changed from: 'sto1*' to: 'stn1*'
- [C] 'function void fn4(sto2*)' at test-leaf3-v1.c:19:1 has some sub-type changes:
+ [C] 'function void fn4(sto2*)' at test-leaf-redundant-v1.c:21:1 has some sub-type changes:
parameter 1 of type 'sto2*' changed:
pointer type changed from: 'sto2*' to: 'stn2*'
diff --git a/tests/data/test-abidiff-exit/test-leaf-redundant-v0.c b/tests/data/test-abidiff-exit/test-leaf-redundant-v0.c
index 1d9c6266..c5dc8216 100644
--- a/tests/data/test-abidiff-exit/test-leaf-redundant-v0.c
+++ b/tests/data/test-abidiff-exit/test-leaf-redundant-v0.c
@@ -1,3 +1,4 @@
+// See comments in -v1.c file.
struct sto1 {
int x;
};
diff --git a/tests/data/test-abidiff-exit/test-leaf-redundant-v0.o b/tests/data/test-abidiff-exit/test-leaf-redundant-v0.o
index 1e74a51b929f63ba77041febe2b2aa064e41cc78..ef7df991deb2c7337b13d008a2977c0e5fdd37de 100644
GIT binary patch
delta 594
zcmZ`$K~L0B5T0qb?Q3_JP&aH-G_VqbV6scQkgR*z)fh1zAV@;+1QOT+$x=`%di7wu
zx|xG+xbPE9xSHtQi+l0zKY(yCF(k|^4F?jM&dm4qd*AfEdH?*c!=EScWoPo>k)~z=
zKz|RYTY>;G8Dl$6KohLcR1sIvtv&)Ulp&HC#WR2fa<MSlWp3>=^8{&~El_Iia?Ld0
zAg{UY4SK(>wDP4^1<Z@zKW8rfd{JEDx1%@&bZ8en&Y$ql^Ho-KnBt81e<05pQHZ=r
zYd<NsTWeAIO>5(Ays;K1<%6KI&N}bc2fKe5A-aupsNE|*n_ntD32UW#rCJF~)gY{g
zVNj<ttufx-jW<g&fvIF?@5a^?=*6M><h2I!i{oXzhyF05Te7?Kand?Mj}6ZsV<zfQ
z`Zb%@ae8gs9ld@EnMD*=Ii@*g=&xCz@5UWEF_i13PU&xRwCU9FaFcBF9j?#^^9we}
zHNE4&dZpoS>exRtWznKDi|f>NYWR?PPSgIG4(KEK*&05j7uhDZsl$1XdYl)?%{4WZ
Wp~YOoKI-2;zys>$YWR(Mx%(I1wQ!*T
delta 575
zcmZ8eF;Cl25WaI_$9CKz1I0Kk!l)5VhJ-vP2<VVdr3y=_I<yiaw2E6KaDYUCp?k{)
z+(0|BFc(Fg`vYKMVPfdmvDqpiB<|T!2cF)2clW;U-n+BEW&XJbIh;StcNbA_06=07
zP)W%MAemIx!wx7i)=||1_LA442td_UNohe?0URb9M<%~>o@#UE$VfT-1iHQ)Ys!E_
zta<DVIp_bmLd^MqmaKPgIhXa;n5>fZMi2@>gIee?e!;8iTV@h4&4_sO@Bt&DOyuq~
z+nvHjb1f(rx~1X=KIS%EFrpb`lA%*2Te&aWXoVj=IqOVka|@PUzw_#1&RZz@MdA4+
zu^>drr{79-8kXwy=U(n*^Zoi-7z+wwE`LwjYb7obZF-?P?jwG5m^(A&>%7vj1DCc{
z2PbG>of;c7D<=&?pRLSRIZBPvwfe-(4Jzz`mp2(~CfS;UPbshMVunt%W1J<SIlVWj
zRRw?0nej{cqtTApz!kESBXnVAah&>Q&G<R^9G|1}SLrMY+D_M4cNSrvu|&eEDSs2R
VV^xi>(T@h0iZ8OVctCyY*&W5DYb*c&
diff --git a/tests/data/test-abidiff-exit/test-leaf-redundant-v1.c b/tests/data/test-abidiff-exit/test-leaf-redundant-v1.c
index fe182cd8..1763a332 100644
--- a/tests/data/test-abidiff-exit/test-leaf-redundant-v1.c
+++ b/tests/data/test-abidiff-exit/test-leaf-redundant-v1.c
@@ -1,3 +1,5 @@
+// Ensure that --redundant is indeed implied by --leaf-changes-only mode and
+// that all the changed functions get complete reports.
struct stn1 {
int x;
};
@@ -7,14 +9,14 @@ struct stn2 {
double y;
};
-void fn1(struct stn1 s) {
+void fn1(struct stn1 s) { // was sto1
}
-void fn2(struct stn2 s) {
+void fn2(struct stn2 s) { // was sto2
}
-void fn3(struct stn1* s) {
+void fn3(struct stn1* s) { // was sto1
}
-void fn4(struct stn2* s) {
+void fn4(struct stn2* s) { // was sto2
}
diff --git a/tests/data/test-abidiff-exit/test-leaf-redundant-v1.o b/tests/data/test-abidiff-exit/test-leaf-redundant-v1.o
index f7db3a0cd5c0d44e14a1d00e7503a07e50a7e1cd..443777797e4a5fc694cda2cfa46544ab639406c1 100644
GIT binary patch
delta 617
zcmZuuL1+_E5S`i0Cc8;$n@IXiw#Zh3O@&SJBPj9EAQ9umLP1YnlDZoW#%;A*sdx~3
z@uJZ09IQtVo;>y<dJu13bCF&|ynFB{h~R9p6bXL*e=~pH{5LcFccsJ9UIq8n^6oj^
zybJ*P4qze`c>sxoF$hA?B7Y83<~_C%Z;n0C9|BtHyNw0)#})<gmT67h2GhLCUa}vY
z1<X?oZIKsWk2rV2H2`O+hjwmuKFZcq7p#6D0kiZR1>+n1QtL9KLyZyf_pup9#OM<3
z(BBH(q2KGf_kC|W@OnW=ueB4Hq4(Otd}sB#)0XF*X0=h3PNOcHQr4Sf>nonOvAsU@
zVYm_8a-#8S;oZ&uskski^(JhE+!R9tpwn(&a4MJl^+7L~2)nYmJXM0*<8|;@A0d`#
z%@D<te9v>k=`jN!Vtb077y?hzD`PSJ&kP@o1v)e;2!u&d9K^|m*hV52ACF|RQ{0WC
zt5fV#QvCKt9DOv!UZYRRm$*U~Q{rGV)z$D9jjWG&g;ctO9xW!0(~oo+cPN)>S>MLL
u5AbXpA7;vUhg7D8FKEQLPq}Oh_h^l=PAc27J{yy2XC^w?GXA98%+g;V0(=?(
delta 613
zcmZ9IK~EDw6vyA&WwyKB*0ho`Yzt<45L=VB%r??sB0WIp2@_9hyil}NlDa@x48(&d
zXA?H>z$f6u#SIs4eg(Y{4idsg;6Ml=CeF00hi+#7@BiNK&FtGb&417DmvB2Qf0;pZ
z5didUKrNvhz{m(&^#jn9&mc25WK`VfJJ5Fk6X8VxN5ek_KFW_I*1~sSnoH6dUB3>P
zp&Ax=hM)asl>zer)6~X7Ha(`gwQw1%t}g(S^cA_(Lp=HPToN6oq$1wlNlQgkiCRHt
zGpO`B?QXsDK~!JMG5~YH>QAc@-9m<2%$Z|oK4Ezo_6Dly@}S}En}P2^Z_R(}s$YJy
zwzbmh*iZ}JuY*Ja=v>R8M_1b16tw2&AG@W6&dO@rcW0|!Ra8WyJaDczEoOz7_Kpwq
zU+84zAa~Wk@so^Wjy^N)I5JbT7+38$PEy2*Sf}4?g05K^Yg9BixrUj|#U1J#OTPE%
znbC3{#?_}oG((ZWoqu~i4$*sbVI1HiYA5*deqvd}Q@XT%V2Pq+3t!PqvVkv1*iGvs
szSBM0u^af3BD;yZblF#tYT`O=NFApr)wI5`!B>iOlWO2k66ptj0k7t5i2wiq
diff --git a/tests/data/test-abidiff-exit/test-leaf-stats-v0.cc b/tests/data/test-abidiff-exit/test-leaf-stats-v0.cc
index 27ba39c9..12a00e0a 100644
--- a/tests/data/test-abidiff-exit/test-leaf-stats-v0.cc
+++ b/tests/data/test-abidiff-exit/test-leaf-stats-v0.cc
@@ -1,3 +1,4 @@
+// See comments in -v1.c file.
int changed_var = 0;
int changed_fun() {
diff --git a/tests/data/test-abidiff-exit/test-leaf-stats-v0.o b/tests/data/test-abidiff-exit/test-leaf-stats-v0.o
index a79511cc3c6db039aaa1f5528db19b55f75c7a2d..2faf8314022d140d44519fdc997fa601fc171faf 100644
GIT binary patch
delta 396
zcmY+8K}!N*5Qe|m{nqwdB=BH*h+2zityWzTmhGU0!3IInA*mp2wbhIg(M<>n^b(yy
znVZn5Um#xcRM-B6-GVM%y|u6iH83+g!^`{5IQSFnDv;<!^T74m^FVvgeUwGY^JD|l
zu;5dhj^#A*6v9hELE01GnBxe5xqm2U*Vna_Y{Zf=O-?2>L(}jbwk}H-R;^;67g|<>
z#GUIpaqOm}UfEXJ^HN)hSgAw`Cz>{umTzh;@&^bpv)N5q*|CdNtL8<hni2gMmN15C
zt^p9j0Uv^8{NN+gZ@>8!AUI3ca7@KOfOsXji@9CF0~&%NR;dYj9MCvC;ir4K#|h2h
zEA`_Y^$*X4Fb6W;&b)zwhG;?#JE9IpI1)4L0X)k-0@Inyd9;e#Oy^7vBQS?urob+a
eSO)4iac3J1DZ{>aPnyDxq_aoCqZjP>7JmV|$zwAB
delta 404
zcmew$`apDo2BX46%`isBi48K0%oFF%Vs~R;U|?oYpKQk{mtvq>W}ugx3>0BtU=?QI
zWCF8*ga|8_H!EW#6B7d?V{%4fUV3Uud|GK91B_jkSj140T3n)=lbV<YGsWF6RKZzW
z+t5V8QqM@wP{GK+(9*zi@*GA7QT>wK0*3e~LqnJWK*P!=%QK0JSZJcCHn0TRzS)hb
zh*44q$PxrP6$V(K47te{nH3ogCjVeoovgs3A*l%!7lhJWK$;VXB_=1bh%?qtu4Hj%
z<e7YsMW2yj@<*UZ%VbVge@3&(jV$7u3s@zX7zHMGvhQb<nC!{n&geCHB8NGn$K;b7
z_MDB35EG|P7UVSN6oUqi+T=h^b0!<6$%>3(lNWGmFgi>=$!X7+FqxCfp0Q%GCzn0v
X1E{0*Cr{)u=d56b7_)ftL^gQ<yL3{)
diff --git a/tests/data/test-abidiff-exit/test-leaf-stats-v1.cc b/tests/data/test-abidiff-exit/test-leaf-stats-v1.cc
index 020cb761..192d672a 100644
--- a/tests/data/test-abidiff-exit/test-leaf-stats-v1.cc
+++ b/tests/data/test-abidiff-exit/test-leaf-stats-v1.cc
@@ -1,5 +1,7 @@
-long changed_var = 0;
+// Test to ensure changed variables and functions are accounted for correctly in
+// --leaf-changes-only mode.
+long changed_var = 0; // was int
-long changed_fun() {
+long changed_fun() { // was int
return 0;
}
diff --git a/tests/data/test-abidiff-exit/test-leaf-stats-v1.o b/tests/data/test-abidiff-exit/test-leaf-stats-v1.o
index 4a433b9572abb46799391619f21538b54fb3204c..4f2288c96881d79a5fc449679338861170061d08 100644
GIT binary patch
delta 477
zcmZ8eJxjw-6n!^&sYy^lgjN?xF;c&h_7#_k5TQbbf)!Cv7kSoLg&M`CMM1$y#L038
zi(Byz2wes3=3j8LKfz6K@ugK!5AUAKxg5^>x{TJN*W<8Oo7fnUBUwU-CJX97R+X^R
zkU*(5Zt*oj6Ec|&HGK5<2?hv7xB{;nSS63|Sml5RW#lkqM-@G%z`7&}XJME7MQ?kv
z%8!V<xN2mlrr3;;vC}p)(kYf<3|lZ?{1;<YnO!K5WUzloN(X+?*z*Ijxxv_X+)l->
zl3#)lH=CU`#uvP;J??k&EtZ-1vr{V5%t;l17`EgXjN*$Nk9-~bLmmTyBXkm<s5&qZ
z-d#W!rHR@?PGfM4C)5?L7EQt#zR}q5O=uEW?zSKQ1;&*;KGWXjsbWfy!bZ;{EFo82
z$YV_!#0S-cb8M@Qb``#)z+GoPr<s~1btj3=<Ww_Z1sj?JYuFZC$GGmmHFDk2-oO5A
NrkkmoaF1<0@d?6$Zq)z)
delta 482
zcmbOs)*&`QgVAH6W|SNwn==Cg11B?(00U+YRuIhsA{dz`E}T{G24pcasDt>7zxjB9
zBqJk;141RK#U;8qsflR@x@CrX$;m)*1_o9U22Lh03rL8ta(S~dMlvx0^(AK{=B1~m
z#HW?!F~HbmiA4<hCAkF*Ir(|%3YmE&4DNoR3eMWvh9(M@dPaJN><UH(hL#4Flh-gh
zc*RE<8p3n}O)10T0C#6+D+LXg)TGSBJctRp7Mc(S)FcB-pu08)GnFz*3ISPyK;OXt
z2b3W<`606+qrqej7S+iHEE<w(P;o&h%>|@6fw*^aA&WSp#N<vEcSfGc7g_WfWhXPT
zich}4!XX%hrnY9XA*(;5+2%%8eI`bQ$s5`CGdfHz<Zx&7n!J<4oUvf?M-F?=R#4Ca
z!Mw?qoaUU$&|p-XT*+z9=s9^Kr#)l9<c~nIW3nL^h%Dr?=llS5zy9Q%T;_}mCV%8o
F2LLeTULODe
--
2.26.0.110.g2183baf09c-goog
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Improve readability of represent helper function.
2020-04-08 12:20 [PATCH 1/2] Improve readability of represent helper function Giuliano Procida
2020-04-08 12:20 ` [PATCH 2/2] abidiff: Document and refresh tests Giuliano Procida
@ 2020-04-08 12:47 ` Matthias Maennich
2020-04-08 14:24 ` Giuliano Procida
1 sibling, 1 reply; 10+ messages in thread
From: Matthias Maennich @ 2020-04-08 12:47 UTC (permalink / raw)
To: Giuliano Procida; +Cc: libabigail, dodji, kernel-team
On Wed, Apr 08, 2020 at 01:20:43PM +0100, Giuliano Procida wrote:
>This change:
>
> - makes local variables const where possible
> - gives local variables more descriptive names
> - factors out some more expressions as local variables
> - simplifies the logic around anonymous data members
>
>There are no behavioural changes.
>
> * src/abg-reporter-priv.cc (represent): In the var_diff_sptr
> overload, rename pretty_representation to
> o_pretty_representation, introduce n_pretty_representation
> where needed and replace the duplicate tr1 and tr2 with these;
> rename all other variables foo1 and foo2 to o_foo and n_foo
> respectively, using _type instead of _t; introduce o_anon and
> n_anon and use them to make the local variable
> is_strict_anonymous_data_member_change const, make ABG_ASSERT
> in anonymous data member handling more obvious in the case
> where anonymity has changed and allow simplification of
> formatting in this case; move declarations of const local
> variables above those of the non-const, state-tracking,
> variables.
>
>Signed-off-by: Giuliano Procida <gprocida@google.com>
>---
> src/abg-reporter-priv.cc | 117 +++++++++++++++++----------------------
> 1 file changed, 50 insertions(+), 67 deletions(-)
>
>diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc
>index 5fb5fbc5..a46c0e9d 100644
>--- a/src/abg-reporter-priv.cc
>+++ b/src/abg-reporter-priv.cc
>@@ -409,14 +409,21 @@ represent(const var_diff_sptr &diff,
> if (!ctxt->get_reporter()->diff_to_be_reported(diff.get()))
> return;
>
>- var_decl_sptr o = diff->first_var();
>- var_decl_sptr n = diff->second_var();
>- string name1 = o->get_qualified_name();
>- string name2 = n->get_qualified_name();
>- uint64_t size1 = get_var_size_in_bits(o);
>- uint64_t size2 = get_var_size_in_bits(n);
>- uint64_t offset1 = get_data_member_offset(o);
>- uint64_t offset2 = get_data_member_offset(n);
>+ const var_decl_sptr o = diff->first_var();
>+ const var_decl_sptr n = diff->second_var();
>+ const bool o_anon = !!is_anonymous_data_member(o);
>+ const bool n_anon = !!is_anonymous_data_member(n);
>+ const bool is_strict_anonymous_data_member_change = o_anon && n_anon;
>+ const string o_name = o->get_qualified_name();
>+ const string n_name = n->get_qualified_name();
>+ const uint64_t o_size = get_var_size_in_bits(o);
>+ const uint64_t n_size = get_var_size_in_bits(n);
>+ const uint64_t o_offset = get_data_member_offset(o);
>+ const uint64_t n_offset = get_data_member_offset(n);
>+ const string o_pretty_representation = o->get_pretty_representation();
Any reason to not have n_pretty_representation also already defined
here?
Otherwise nice cleanup!
Reviewed-by: Matthias Maennich <maennich@google.com>
Cheers,
Matthias
>+
>+ const bool show_size_offset_changes = ctxt->get_allowed_category()
>+ & SIZE_OR_OFFSET_CHANGE_CATEGORY;
>
> // Has the main diff text been output?
> bool emitted = false;
>@@ -424,39 +431,31 @@ represent(const var_diff_sptr &diff,
> bool begin_with_and = false;
> // Have we reported a size change already?
> bool size_reported = false;
>- // Are we reporting a change to an anonymous member?
>- bool is_strict_anonymous_data_member_change = false;
>
>- string pretty_representation = o->get_pretty_representation();
>- bool show_size_offset_changes = ctxt->get_allowed_category()
>- & SIZE_OR_OFFSET_CHANGE_CATEGORY;
>-
>- if (is_anonymous_data_member(o) && is_anonymous_data_member(n))
>+ if (is_strict_anonymous_data_member_change)
> {
>- is_strict_anonymous_data_member_change = true;
>- string tr1 = o->get_pretty_representation();
>- string tr2 = n->get_pretty_representation();
>- type_base_sptr t1 = o->get_type(), t2 = n->get_type();
>- if (tr1 != tr2)
>+ const string n_pretty_representation = n->get_pretty_representation();
>+ const type_base_sptr o_type = o->get_type(), n_type = n->get_type();
>+ if (o_pretty_representation != n_pretty_representation)
> {
> show_offset_or_size(indent + "anonymous data member at offset",
>- offset1, *ctxt, out);
>+ o_offset, *ctxt, out);
>
> out << " changed from:\n"
>- << indent << " " << tr1 << "\n"
>+ << indent << " " << o_pretty_representation << "\n"
> << indent << "to:\n"
>- << indent << " " << tr2 << "\n";
>+ << indent << " " << n_pretty_representation << "\n";
>
> begin_with_and = true;
> emitted = true;
> }
>- else if (get_type_name(t1) != get_type_name(t2)
>- && is_decl(t1) && is_decl(t2)
>- && is_decl(t1)->get_is_anonymous()
>- && is_decl(t2)->get_is_anonymous())
>+ else if (get_type_name(o_type) != get_type_name(n_type)
>+ && is_decl(o_type) && is_decl(n_type)
>+ && is_decl(o_type)->get_is_anonymous()
>+ && is_decl(n_type)->get_is_anonymous())
> {
> out << indent << "while looking at anonymous data member '"
>- << tr1 << "':\n"
>+ << o_pretty_representation << "':\n"
> << indent << "the internal name of that anonymous data member"
> "changed from:\n"
> << indent << " " << get_type_name(o->get_type()) << "\n"
>@@ -472,36 +471,20 @@ represent(const var_diff_sptr &diff,
> }
> else if (filtering::has_anonymous_data_member_change(diff))
> {
>+ ABG_ASSERT(o_anon != n_anon);
> // So we are looking at a non-anonymous data member change from
> // or to anonymous data member.
>- if (is_anonymous_data_member(o))
>- {
>- string repr = "anonymous data member "
>- + o->get_pretty_representation()
>- + " at offset";
>-
>- show_offset_or_size(indent + repr, offset1, *ctxt, out);
>- out << " became data member '"
>- << n->get_pretty_representation()
>- << "'\n";
>- }
>- else
>- {
>- ABG_ASSERT(is_anonymous_data_member(n));
>- string repr = "data member "
>- + o->get_pretty_representation()
>- + " at offset";
>-
>- show_offset_or_size(indent + repr, offset1, *ctxt, out);
>- out << " became anonymous data member '"
>- << n->get_pretty_representation()
>- << "'\n";
>- }
>+ const string n_pretty_representation = n->get_pretty_representation();
>+ out << indent << (o_anon ? "anonymous " : "")
>+ << "data member " << o_pretty_representation;
>+ show_offset_or_size(" at offset", o_offset, *ctxt, out);
>+ out << " became " << (n_anon ? "anonymous " : "")
>+ << "data member '" << n_pretty_representation << "'\n";
>
> begin_with_and = true;
> emitted = true;
> }
>- else if (diff_sptr d = diff->type_diff())
>+ else if (const diff_sptr d = diff->type_diff())
> {
> if (ctxt->get_reporter()->diff_to_be_reported(d.get()))
> {
>@@ -511,7 +494,7 @@ represent(const var_diff_sptr &diff,
> << "' changed";
> else
> out << indent
>- << "type of '" << pretty_representation << "' changed";
>+ << "type of '" << o_pretty_representation << "' changed";
>
> if (d->currently_reporting())
> out << ", as being reported\n";
>@@ -529,7 +512,7 @@ represent(const var_diff_sptr &diff,
> }
> }
>
>- if (!filtering::has_anonymous_data_member_change(diff) && name1 != name2)
>+ if (!filtering::has_anonymous_data_member_change(diff) && o_name != n_name)
> {
> if (filtering::has_harmless_name_change(o, n)
> && !(ctxt->get_allowed_category()
>@@ -546,7 +529,7 @@ represent(const var_diff_sptr &diff,
> out << indent;
> else
> out << ", ";
>- out << "name of '" << name1 << "' changed to '" << name2 << "'";
>+ out << "name of '" << o_name << "' changed to '" << n_name << "'";
> report_loc_info(n, *ctxt, out);
> emitted = true;
> }
>@@ -561,7 +544,7 @@ represent(const var_diff_sptr &diff,
> begin_with_and = false;
> }
> else if (!emitted)
>- out << indent << "'" << pretty_representation << "' ";
>+ out << indent << "'" << o_pretty_representation << "' ";
> else
> out << ", ";
> if (get_data_member_is_laid_out(o))
>@@ -572,7 +555,7 @@ represent(const var_diff_sptr &diff,
> }
> if (show_size_offset_changes)
> {
>- if (offset1 != offset2)
>+ if (o_offset != n_offset)
> {
> if (begin_with_and)
> {
>@@ -584,17 +567,17 @@ represent(const var_diff_sptr &diff,
> out << indent;
> if (is_strict_anonymous_data_member_change)
> out << "anonymous data member ";
>- out << "'" << pretty_representation << "' ";
>+ out << "'" << o_pretty_representation << "' ";
> }
> else
> out << ", ";
>
>- show_numerical_change("offset", offset1, offset2, *ctxt, out);
>+ show_numerical_change("offset", o_offset, n_offset, *ctxt, out);
> maybe_show_relative_offset_change(diff, *ctxt, out);
> emitted = true;
> }
>
>- if (!size_reported && size1 != size2)
>+ if (!size_reported && o_size != n_size)
> {
> if (begin_with_and)
> {
>@@ -606,12 +589,12 @@ represent(const var_diff_sptr &diff,
> out << indent;
> if (is_strict_anonymous_data_member_change)
> out << "anonymous data member ";
>- out << "'" << pretty_representation << "' ";
>+ out << "'" << o_pretty_representation << "' ";
> }
> else
> out << ", ";
>
>- show_numerical_change("size", size1, size2, *ctxt, out);
>+ show_numerical_change("size", o_size, n_size, *ctxt, out);
> maybe_show_relative_size_change(diff, *ctxt, out);
> emitted = true;
> }
>@@ -624,7 +607,7 @@ represent(const var_diff_sptr &diff,
> begin_with_and = false;
> }
> else if (!emitted)
>- out << indent << "'" << pretty_representation << "' ";
>+ out << indent << "'" << o_pretty_representation << "' ";
> else
> out << ", ";
> out << "elf binding changed from " << o->get_binding()
>@@ -639,7 +622,7 @@ represent(const var_diff_sptr &diff,
> begin_with_and = false;
> }
> else if (!emitted)
>- out << indent << "'" << pretty_representation << "' ";
>+ out << indent << "'" << o_pretty_representation << "' ";
> else
> out << ", ";
> out << "visibility changed from " << o->get_visibility()
>@@ -656,7 +639,7 @@ represent(const var_diff_sptr &diff,
> begin_with_and = false;
> }
> else if (!emitted)
>- out << indent << "'" << pretty_representation << "' ";
>+ out << indent << "'" << o_pretty_representation << "' ";
> else
> out << ", ";
>
>@@ -675,7 +658,7 @@ represent(const var_diff_sptr &diff,
> begin_with_and = false;
> }
> else if (!emitted)
>- out << indent << "'" << pretty_representation << "' ";
>+ out << indent << "'" << o_pretty_representation << "' ";
> else
> out << ", ";
>
>@@ -691,7 +674,7 @@ represent(const var_diff_sptr &diff,
> ;
> else if (!emitted)
> // We appear to have fallen off the edge of the map.
>- out << indent << "'" << pretty_representation
>+ out << indent << "'" << o_pretty_representation
> << "' has *some* difference - please report as a bug";
> else
> // do nothing
>--
>2.26.0.110.g2183baf09c-goog
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] abidiff: Document and refresh tests.
2020-04-08 12:20 ` [PATCH 2/2] abidiff: Document and refresh tests Giuliano Procida
@ 2020-04-08 12:54 ` Matthias Maennich
2020-04-14 11:24 ` Dodji Seketeli
1 sibling, 0 replies; 10+ messages in thread
From: Matthias Maennich @ 2020-04-08 12:54 UTC (permalink / raw)
To: Giuliano Procida; +Cc: libabigail, dodji, kernel-team
On Wed, Apr 08, 2020 at 01:20:44PM +0100, Giuliano Procida wrote:
>Following on from giving some test file more descriptive names, this
>patch actually documents in brief the purpose of these recently added
>tests.
>
>It also improves the coverage of the test-leaf-cxx-members test to
>include deletion and insertion report text as well.
>
> * tests/data/test-abidiff-exit/test-leaf-cxx-members-v1.cc:
> Comment test. Reorder members of ops to get better coverage.
> * tests/data/test-abidiff-exit/test-leaf-fun-type-v1.cc:
> Comment test.
> * tests/data/test-abidiff-exit/test-leaf-more-v1.cc: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc:
> Comment test. Update comment on ops2.
> * tests/data/test-abidiff-exit/test-leaf-redundant-v1.c:
> Comment test.
> * tests/data/test-abidiff-exit/test-leaf-stats-v1.cc: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-cxx-members-report.txt:
> Update locations. Update report to reflect deletion,
> insertion and changed members (was previously changed only).:
> * tests/data/test-abidiff-exit/test-leaf-peeling-report.txt:
> Update locations.
> * tests/data/test-abidiff-exit/test-leaf-redundant-report.txt:
> Ditto.:
> * tests/data/test-abidiff-exit/test-leaf-cxx-members-v0.cc:
> Added one line comment referring to -v1 source file.
> * tests/data/test-abidiff-exit/test-leaf-fun-type-v0.cc: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-more-v0.cc: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-redundant-v0.c: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-stats-v0.cc: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-cxx-members-v0.o: Recompiled.
> * tests/data/test-abidiff-exit/test-leaf-cxx-members-v1.o: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-fun-type-v0.o: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-fun-type-v1.o: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-more-v0.o: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-more-v1.o: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-peeling-v0.o: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-peeling-v1.o: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-redundant-v0.o: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-redundant-v1.o: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-stats-v0.o: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-stats-v1.o: Ditto.
>
That really improves readability of those tests and will make future
troubleshooting easier! Thanks a lot!
Reviewed-by: Matthias Maennich <maennich@google.com>
Cheers,
Matthias
>Signed-off-by: Giuliano Procida <gprocida@google.com>
>---
> .../test-leaf-cxx-members-report.txt | 17 +++++++-------
> .../test-leaf-cxx-members-v0.cc | 6 ++---
> .../test-leaf-cxx-members-v0.o | Bin 7328 -> 7328 bytes
> .../test-leaf-cxx-members-v1.cc | 13 +++++++----
> .../test-leaf-cxx-members-v1.o | Bin 7360 -> 7368 bytes
> .../test-leaf-fun-type-v0.cc | 1 +
> .../test-abidiff-exit/test-leaf-fun-type-v0.o | Bin 2840 -> 2864 bytes
> .../test-leaf-fun-type-v1.cc | 4 +++-
> .../test-abidiff-exit/test-leaf-fun-type-v1.o | Bin 2952 -> 2976 bytes
> .../test-abidiff-exit/test-leaf-more-v0.cc | 1 +
> .../test-abidiff-exit/test-leaf-more-v0.o | Bin 3792 -> 3800 bytes
> .../test-abidiff-exit/test-leaf-more-v1.cc | 8 ++++---
> .../test-abidiff-exit/test-leaf-more-v1.o | Bin 3808 -> 3832 bytes
> .../test-leaf-peeling-report.txt | 12 +++++-----
> .../test-abidiff-exit/test-leaf-peeling-v0.cc | 1 +
> .../test-abidiff-exit/test-leaf-peeling-v0.o | Bin 4528 -> 4528 bytes
> .../test-abidiff-exit/test-leaf-peeling-v1.cc | 22 ++++++++++--------
> .../test-abidiff-exit/test-leaf-peeling-v1.o | Bin 4600 -> 4600 bytes
> .../test-leaf-redundant-report.txt | 10 ++++----
> .../test-leaf-redundant-v0.c | 1 +
> .../test-leaf-redundant-v0.o | Bin 3320 -> 3352 bytes
> .../test-leaf-redundant-v1.c | 10 ++++----
> .../test-leaf-redundant-v1.o | Bin 3384 -> 3416 bytes
> .../test-abidiff-exit/test-leaf-stats-v0.cc | 1 +
> .../test-abidiff-exit/test-leaf-stats-v0.o | Bin 2784 -> 2800 bytes
> .../test-abidiff-exit/test-leaf-stats-v1.cc | 6 +++--
> .../test-abidiff-exit/test-leaf-stats-v1.o | Bin 2824 -> 2840 bytes
> 27 files changed, 65 insertions(+), 48 deletions(-)
>
>diff --git a/tests/data/test-abidiff-exit/test-leaf-cxx-members-report.txt b/tests/data/test-abidiff-exit/test-leaf-cxx-members-report.txt
>index eae74a3b..27fdc45c 100644
>--- a/tests/data/test-abidiff-exit/test-leaf-cxx-members-report.txt
>+++ b/tests/data/test-abidiff-exit/test-leaf-cxx-members-report.txt
>@@ -15,28 +15,27 @@ Removed/Changed/Added variables summary: 0 Removed, 0 Changed, 0 Added variable
>
> 1 function with some sub-type change:
>
>- [C] 'method virtual int ops::changed_fn()' at test-leaf-cxx-members-v1.cc:6:1 has some sub-type changes:
>+ [C] 'method virtual int ops::changed_fn()' at test-leaf-cxx-members-v1.cc:5:1 has some sub-type changes:
> return type changed:
> type name changed from 'int' to 'long int'
> type size changed from 32 to 64 (in bits)
>
>-'struct ops at test-leaf-cxx-members-v0.cc:1:1' changed:
>+'struct ops at test-leaf-cxx-members-v0.cc:2:1' changed:
> type size changed from 128 to 192 (in bits)
> 1 member function deletion:
>- 'method virtual int ops::deleted_fn()' at test-leaf-cxx-members-v0.cc:3:1, virtual at voffset 0/1 {_ZN3ops10deleted_fnEv}
>+ 'method virtual int ops::deleted_fn()' at test-leaf-cxx-members-v0.cc:6:1, virtual at voffset 1/1 {_ZN3ops10deleted_fnEv}
> 1 member function insertion:
>- 'method virtual long int ops::added_fn()' at test-leaf-cxx-members-v1.cc:3:1, virtual at voffset 0/1 {_ZN3ops8added_fnEv}
>+ 'method virtual long int ops::added_fn()' at test-leaf-cxx-members-v1.cc:11:1, virtual at voffset 1/1 {_ZN3ops8added_fnEv}
> there are member function changes:
> 'method virtual int ops::changed_fn()' has some changes:
> return type changed:
> type name changed from 'int' to 'long int'
> type size changed from 32 to 64 (in bits)
>+ 1 data member deletion:
>+ 'int ops::deleted_var', at offset 96 (in bits) at test-leaf-cxx-members-v0.cc:5:1
>+ 1 data member insertion:
>+ 'long int ops::added_var', at offset 128 (in bits) at test-leaf-cxx-members-v1.cc:10:1
> there are data member changes:
>- type 'int' of 'ops::deleted_var' changed:
>- type name changed from 'int' to 'long int'
>- type size changed from 32 to 64 (in bits)
>- and name of 'ops::deleted_var' changed to 'ops::added_var' at test-leaf-cxx-members-v1.cc:2:1
> type 'int' of 'ops::changed_var' changed:
> type name changed from 'int' to 'long int'
> type size changed from 32 to 64 (in bits)
>- and offset changed from 96 to 128 (in bits) (by +32 bits)
>diff --git a/tests/data/test-abidiff-exit/test-leaf-cxx-members-v0.cc b/tests/data/test-abidiff-exit/test-leaf-cxx-members-v0.cc
>index 89612562..e145375e 100644
>--- a/tests/data/test-abidiff-exit/test-leaf-cxx-members-v0.cc
>+++ b/tests/data/test-abidiff-exit/test-leaf-cxx-members-v0.cc
>@@ -1,9 +1,9 @@
>+// See comments in -v1.c file.
> struct ops {
>- int deleted_var;
>- virtual int deleted_fn() { return 0; }
>-
> int changed_var;
> virtual int changed_fn() { return 0; }
>+ int deleted_var;
>+ virtual int deleted_fn() { return 0; }
> };
>
> void reg(ops& x) {
>diff --git a/tests/data/test-abidiff-exit/test-leaf-cxx-members-v0.o b/tests/data/test-abidiff-exit/test-leaf-cxx-members-v0.o
>index 8202ecb5dd7cb2bbe79a0a2f3ff42a103f56387c..cf5c9eb87a1dd22db9e7f0532de1530362946c69 100644
>GIT binary patch
>delta 302
>zcmZ2rxxjKmEF&Y+<T%C_K4x|$Mg|5BP6i-gWSRVd@dhK)=95fr%#5Ow1=tK3g(o|)
>zrB43K)*#{T7pmZ_t!-$cV5w)MXQ*IgU}$M*Xka;cKbzg;T6S+iOHDMH%{SRiSSBm;
>zh)lL%=ih9=<G{#xbaElDxMU#6HXsm%Pz+o^S^$XqCokl6mplL!wTIF$+1$yDeDR#C
>zprRWlC-RC5?nP50KY1dbxa1V5=wme5Et3!OiBF!u$07I~O{Q}5M?QCE*5r)Dy!6So
>QLgJIH*m*Z63fXf30L}R}<p2Nx
>
>delta 320
>zcmZ2rxxjKmEF&Z1<T%C_J|=c0Mg|5BP6i-gWS;zi@dhK~=95fr%#0$F1=tLMs+`zT
>zCEWc&6`ZxT4NVj*^^Eik6^slFEe#C~j3(Q&+f821)-bu2-CNLH6HRvWO?DHO$;vz;
>zlg)(qHyiLcFfzJMF60%LTn@4g2t*+i0~e4M0OF^U7xKDGiUA!20`?FJE_-+~BVW8B
>z15|bcnk*Mod@q`~*5rwN;*#f~qL0yJXHP!JCq8)sABW&~G?`bMKk~UVv!<lxq?Sy!
>Q6%wCpA;h;iNyv^10JSVM`v3p{
>
>diff --git a/tests/data/test-abidiff-exit/test-leaf-cxx-members-v1.cc b/tests/data/test-abidiff-exit/test-leaf-cxx-members-v1.cc
>index 0437584e..892775fe 100644
>--- a/tests/data/test-abidiff-exit/test-leaf-cxx-members-v1.cc
>+++ b/tests/data/test-abidiff-exit/test-leaf-cxx-members-v1.cc
>@@ -1,11 +1,14 @@
>+// Exercises the reporting of struct differences in --leaf-changes-only mode.
>+// The resulting report should have no excess blank lines.
> struct ops {
>- // TODO: type, name and size are differnent from deleted_var, but this is
>- // still reported as a change rather than a deletion and an addition.
>+ long changed_var; // was int
>+ virtual long changed_fn() { return 0; } // was int
>+ // Note that in order for this to be treated as an addition, as opposed to a
>+ // change, we need to make sure it's at a different offset from anything in
>+ // the old version of ops; having type, name and size different but the same
>+ // offset won't work.
> long added_var;
> virtual long added_fn() { return 0; }
>-
>- long changed_var;
>- virtual long changed_fn() { return 0; }
> };
>
> void reg(ops& x) {
>diff --git a/tests/data/test-abidiff-exit/test-leaf-cxx-members-v1.o b/tests/data/test-abidiff-exit/test-leaf-cxx-members-v1.o
>index 617ffd21551b158227af04c6e601ebc5a12e6b9b..9b74afbffc7868e4549e6cc65fa456b5cd462976 100644
>GIT binary patch
>delta 662
>zcmZ9IOK1~O6o%)_IGIV>%uI$R<4r0}#YaM<$%LW~N{1?#Lah`*&`o0@HVTpiALv5t
>z1B;K1h99Z}UDSo((lVPOh~ma9m2S*ZT<A^|#Nx`Cbc8Yo?tjkt{&V5n`_AfEPjd8N
>zJ%5}?z#QFEcFEJkadMRNf<$c5ekImJTC9iqP~hvQl!)K_kG(-bt1I5!<ZX@=)vx&W
>zM9Q~FmGWNV+tZvX`;~Fcr-J*LmjznI7ja@TB_3E3^XGY<BN8<(%6k*N5Mm_mTM`cj
>zmb}9nc+H#sxW0o4aY8FnmuG9WOSS4;1BWCgUu`z(*(;43n3Bf!+-<h5bComavL_1#
>zb2Mv>lt;{Lxnx?VS+ejWG6qMQOj`Mn<1gd>mW~N@0%qSY+|pGvqyd!rBkt+`QKAo6
>zj2Gz<*5eiSetUP$4zmAuns-`AC{=SKpszx1E$H8b_+dO(4E8vnUqkLVJ}Cwk;&S3o
>z$aw)$i~Jo9Wp##@(N?RT{#1*!><(*9qMI-i20g^MHiS-M#e0&5vpRF{>j{B2;3U7%
>z6c$rY=_8miPv7BqS_0esmM)^}X$cOSr<Y*q3aw!_eS|K<N!zr5uBV$|8JqtrfhdJm
>iCWodWV=Z%(I_PF>dImOV(-tla+R>c8z4{D>+0Z{P`GJW5
>
>delta 651
>zcmZ9I&1(};6vgMhIGIVC&LpYL<fF)pRGScJJ29wH$xyVW2$n7sf(zqdY!oCZZ3!-l
>zl_H2M6At)+T{!*)4Z($iSiwbI3Ek+zg;}|cT}WLBzKJQ69C+v6_dEAJcyBwqm8~b}
>zK{I)dh_FBp6(-M<Ajm#}i#*t)V~W^Cfq^daBE`xVeO`C7ucN(&HlFgcBliUIscy#_
>zvtc))0V&A~Ys?F(>~`9cOR4pZ$sT%+wm6$VERL;<;|0bz7XyNi@rkZ9@%Xx!>;>|H
>zfZj3F9S$8}F|2sYk^J0M{d_8AX7ubtdcxGxQ)bpQr_v}&IsN8syHdVYD%R{Jp?GcX
>zM0IibLT08W*rgJu3l$uaa{jy3N?E^CSrP767x7ZM96XimyQ}qAI_n_|*}+{*`-c&h
>z!mTU*456dQs1GIRE8Y&}qY?fMuB`t>a6d7?*Ly^jVpQy<n|<jt8j2SE+@qiR(p`My
>zX|qQ=ed$U3QZ)JwpE}7@!Ws2C?cjx4<o-vUq@7kWP$gPMBcRbEG=pyjPwzi{riMc3
>zNb5-`=%GzGBR}a99*3XNHVkcze!$_LfD!pX1+=**;6&Hx6^vM(HsHv^SdER*4LC82
>lR?+7E1xDPW->~Br+mrA*o}(?aIjX}LwWtG|yAEe`{2!|-iuV8j
>
>diff --git a/tests/data/test-abidiff-exit/test-leaf-fun-type-v0.cc b/tests/data/test-abidiff-exit/test-leaf-fun-type-v0.cc
>index b75dd84c..db1f7917 100644
>--- a/tests/data/test-abidiff-exit/test-leaf-fun-type-v0.cc
>+++ b/tests/data/test-abidiff-exit/test-leaf-fun-type-v0.cc
>@@ -1,3 +1,4 @@
>+// See comments in -v1.c file.
> struct ops {
> void (*munge)(int x);
> };
>diff --git a/tests/data/test-abidiff-exit/test-leaf-fun-type-v0.o b/tests/data/test-abidiff-exit/test-leaf-fun-type-v0.o
>index 211a33884f30755929210fee5d328e5b7e6c24e1..f03d386fc1156eac6e48a6d7b46f9b0153f4197e 100644
>GIT binary patch
>delta 312
>zcmZ9FF-yZx9K_%M<)!95a1cs^LbbYR#WYEmHVBQlw40l=Qc^ltX*8BPI@`f;OD91G
>zXG<0{xjMMH`w^u50%Ek{;Bdzc_q!Vx9t$^H)Q`)Di`-~2#<Wa~w0N}5bGi5^AHTj~
>zniHDFY&_8FhSj+^wW7;@U=5r~yZzl}`Wln`CuL^hor;?4c=)6S_Jvurqh6my!Fgnz
>z1jn6!1L*Sdl2<Nym1@N;RUOxJGd8*B1ELiyi*@-2U{?fmhC5+W52<L<7^Iu>`uDkq
>zSeU5mEovlJy3J{b$-*;r5gJW8Kx)+Bs62$+ksmZ?ZsExqjSxzo5==6G!c6*@Dg$*@
>Urzt|^%RMn~yGT`yUNBReUwJ`L&Hw-a
>
>delta 357
>zcmdlWHbZQJ24loT%`jd@4i5$f24)~;U}T!uqs7F^HZehD;+f0r&R}u%$;%k!7>y>M
>zWt319V`Xe+VrF1uboUEYaMso~G*PhBGtx6uFfuT-G&C?an9Rmxr&E$zT%wzknwVy!
>zTV|k_oXij(WoT5Cnx0u)l3Em>Ur-$6Z44wC^h<IJCRZ?N3Yux68MAp4QyC+p&}2;(
>zMWD^WEUA)VAol@*D1>6*0@9p7TsQe5i@W3msAw9RZ1ZG8R&mDk$)2qKjAoMqS;aSR
>zV3lBEOqhI<eLrKs<W3HEM)%1VIm{UwCUbJyb3S8)=$|$@kkg#8W^yN|J=0>Q$sd`-
>yCO_cRU|cbolgoZG2dBv77%m;g2a`Lw>^TisAX+phU*t09+`tOqEuMUlO&$QU?N00f
>
>diff --git a/tests/data/test-abidiff-exit/test-leaf-fun-type-v1.cc b/tests/data/test-abidiff-exit/test-leaf-fun-type-v1.cc
>index f308a343..ccf79dbb 100644
>--- a/tests/data/test-abidiff-exit/test-leaf-fun-type-v1.cc
>+++ b/tests/data/test-abidiff-exit/test-leaf-fun-type-v1.cc
>@@ -1,5 +1,7 @@
>+// The change to ops::munge is local to ops and should result in ops being
>+// reported as a changed type in --leaf-changes-only mode.
> struct ops {
>- char (*munge)(long x, bool gunk);
>+ char (*munge)(long x, bool gunk); // everything but the name changed
> };
>
> void register_ops(const ops&) {
>diff --git a/tests/data/test-abidiff-exit/test-leaf-fun-type-v1.o b/tests/data/test-abidiff-exit/test-leaf-fun-type-v1.o
>index 9db3ed5e8f997d9c8455f8361449ce516edc840a..2a23514d593c02f79d432e6ee7c3d5a8aa0b1616 100644
>GIT binary patch
>delta 448
>zcmZ8c%Sr-a6#ma#axO$hhBHAjf>>gsjudEwA_ltBRc%`6DB}f8Z5)F(LDZr}i{!k3
>z?OG;oS_Hm9yP#DmJwb2K)JO{te0*@e%i-L5hu-^bm<~e6%QTlKgwQq!FoU}c<ysn)
>zJ%g_|EV~$*CQ+P#k5_B`wrX8;4g3H}l32K=XaaEBZ`Tb{ZRjScbUH2UQo)pE^enYy
>z=(YchdagyPRbm?TX3sLrQm5M^xx$f>4TrT|B^ilFG$kI>lA0Dv;wF^}l3X^suWTP0
>zm8RZy462q4Em+5|lnJnb8OFb|a0@Rf39pzaoZyJ@%6q@x*|Y8t5p3pd$T(w4_{4CS
>zXWg=Uu9^!j5zkn!T$pcf7E~WTuti9Xzu71SL-cbKc*hx+gCefdYk0s1a42}t;&*r#
>w<zz>H$_pQWXPohA_(Z>uhAHYo8p|Swk3s?@oC#^Z!8i%c=@J8wKwaGW0doRov;Y7A
>
>delta 418
>zcmZ1=-XT6ggYm>f%`jd@4i5$f24)~;U}T!u<H*FyHZehD;^V9A&R}u%$@>}Q)Qxn@
>z4E2(eL8=&7<rz4cz$_pk#>&{t#LU3Rm|L2co;o>^NsQUR*kE!MlbvW%etr%^NosM4
>zZcb`q8q|czVoYil$r*`73`MEwnZ+fkMe+Fs#S9GY&dyc}8ZN0xnTdG{mU>2dhPsAk
>znh=(Pk%6J5A<)>(_n3+p8HFYXvM4edO|E9~m1KjuLl8=H0clPkPMrLZMVzsE@=um{
>z$>~tJY&11RlM7kJC1aqXwP><)CNE?aXKa|flhvQmY%(LO_+|w*2`0vl$(9`Z84pbU
>z$>GlEKG~7eoaqDW<SI@(&i{-M16EEx$Z5{FV)9Q;d#3YDlNU0IO%CAFV7xH7lFNQ_
>l1*gd5Gh8~19Fu=?*>gs)K(uI1cH}naRDk+r@nlDKc>vtxSepO<
>
>diff --git a/tests/data/test-abidiff-exit/test-leaf-more-v0.cc b/tests/data/test-abidiff-exit/test-leaf-more-v0.cc
>index bb6505c8..d67978b1 100644
>--- a/tests/data/test-abidiff-exit/test-leaf-more-v0.cc
>+++ b/tests/data/test-abidiff-exit/test-leaf-more-v0.cc
>@@ -1,3 +1,4 @@
>+// See comments in -v1.c file.
> struct changed {
> int foo = 0;
> };
>diff --git a/tests/data/test-abidiff-exit/test-leaf-more-v0.o b/tests/data/test-abidiff-exit/test-leaf-more-v0.o
>index a8734481238bda41217440b305307ea7c5464782..b3a3084e0e6061e2ac4f7b3ffae6cf81cb519f55 100644
>GIT binary patch
>delta 621
>zcmZ8fzi-n(6u#$+?X#OyH7IdXXyYJMY@)<*1VrvYDg`MKMHGfgAS6miT^gz62aOPH
>z6)Y@BSWo=}s!ox*luk^jTL&bvATg2=w(<uc&W^kEhVOm%z3<(<`!SmPIrqtgv~O+C
>z3amj0p&Q*U0V)TakXTG?cLx5q`IHz43Jp~bVnBfTe=flcN|4oNuK8RM{7s^SsRZ7F
>z9H{sR{>o3dLP;JU3iD#&W%rebJt1AD3W(~m2#>|fc0{h*BD@zU0T_7OgTj{A+z9=J
>z*QH{sMQ~rp<1;CVPbD2M(wy4bY<6CF>!jVmEKTAby_v4xUo~p^ymP~Fi{+wYluM55
>zI3*YF(c258UaKt{<{j^OyV;o-3XW@?vmX7Up8-;MDC(K3ymgH?9}5n}$636BW1e-n
>za)CgY31BK>cTtmcSVv3BMF(D%-r|X5!Wd6^?>)vc$;hkn7+l#XnrHBs>9{1DvmYbn
>zK@`uNKM1~)i?D=;vJI;^=D3R*)8POuW@p|+_V4G)lGtZ9Y~dbv+W3X7z+$k~k)bpM
>z7-4(rEBrz$UWYa8$1D6<hw-X16eg=w@RVyjpQs9Q3^g@{I|&mQ4ii-{@i)gEw5F>{
>VLz)zP!2Wba>4l$xkbUf{+5Z9OcTxZV
>
>delta 586
>zcmZ8eO>0v@6rDToW8Sk#+Sk&?7ioNTlf=*@6Ez`Fj3BgKSqc_)r6EllL0U_L80eyf
>zE~K0KJU>9tKVVuA+$iqcsT=9iozz|3by0D~zHD+A_ndp~xpRk^qsecRFS9ff<~B>@
>z%rnM}mGyN-TxiaiX<CohI?U3XEK7^7p$iq^`*isG5)H$q%ieG%R1y!{%+)=Lp+ZyS
>z;y(QmOG+?U28%qMlGD#hg=SNGm^H>+{-3Fo!ah&oBhP!C)@En=NvpBa_r5a5MYG*l
>zTWu}ljMpyC-B}85=kxM<P%D%RGANg1P0CUkZ;YFGXHVd+QNd4Re5{%qa4>dgn+N2N
>zs)e4qQ$Ngz1o&<x{dClIBDIe<Kz7ofRq7}sHWYRH_z2RTB8hu;S09Y8cFxytqQg4}
>zc0r+jpFBK~<W~q+#VjpiLlqgqo+!`^K8PYcz_BR$zoMzv1Ji#Yog!_a;UqC49BeyT
>z|7qko8Stj?*;$~<q31s2^aUG3pXdlOHm6o}V^wm{i`Ct?(Jx%XsnUKx#_Rmdz>YbA
>g7x64H^x}02aH8^a$VA<}WuLFvMK@8UHhPK7Ke8ofP5=M^
>
>diff --git a/tests/data/test-abidiff-exit/test-leaf-more-v1.cc b/tests/data/test-abidiff-exit/test-leaf-more-v1.cc
>index 5df5df9d..7f5b65d3 100644
>--- a/tests/data/test-abidiff-exit/test-leaf-more-v1.cc
>+++ b/tests/data/test-abidiff-exit/test-leaf-more-v1.cc
>@@ -1,12 +1,14 @@
>+// Ensure that local changes to functions and variables (including their removal
>+// or addition) are reported in --leaf-changes-only mode.
> struct changed {
>- long foo = 0;
>+ long foo = 0; // was int
> };
>
>-long directly_changed_var = 0;
>+long directly_changed_var = 0; // was int
> changed * indirectly_changed_var;
> long added_var = 0;
>
>-long directly_changed_fun() {
>+long directly_changed_fun() { // was int
> return 0;
> }
> void indirectly_changed_fun(changed * x) {
>diff --git a/tests/data/test-abidiff-exit/test-leaf-more-v1.o b/tests/data/test-abidiff-exit/test-leaf-more-v1.o
>index 324180ca92b91045edb4c9d3b9fef1fd314a09ba..4ac71798c2735b828957689acf622d267f60f67b 100644
>GIT binary patch
>delta 685
>zcmZ8fOK1~O6utLNX68+PG=+8&rEQ`Mlg3FhK}aW{0V8dos0~!G?IMiH#1?8IX%Z;9
>z3Ej9%xGUFPxohpJ8>LJ4E_CN->q5HHjrb-REA%byIrp4%;oZmkGWTn4n8c+~YHwZQ
>z6#$T$?KYrq%mI8{78;BTs)`%XBv*0IMCE}Og8x?)Zd`cU=;sVhk%9>fQxX><;Tvw4
>zjkZA*2pFKpc&=^<DnXj6Y+g2=wL1<yXVJXmN7=tB<A%JLmGMBHzCCFf1H`Vg*EMz>
>zyE)DqeKS+910AyK*BUz=r{3M&uhqBh)>EfZ>)RcuZCZAtA)02d)u*a5PZ23bKiE|w
>zsSsUx@F-qRr_Eb&E0fQd@qEs-OfzTEUFptESSgo_@#H<{$&THc1Ps$kU5dKf0G#}h
>z-XccnSk_~+qK}BR_}GC9H0Y~v3qqX+^gMuo$Ep-jbnH<|NqA4Os`MBDy;Bxr-@Q=8
>zL%%@H<%FuvJ_21)mvKb4s>iZkWX;2W(IpzGS=^#CHOoE&ovRr-;+&Fviroh)@=WZt
>zH*t1p@`wgJi!bSz>o`Ln_(LpGLe)=lT7}^!+7G;Epiwcnj5Qhr3z(zRU`cx;?(QCo
>zFjT;AWQ9sh^HIV-OT$o7{EgF42`ltl)K_GMOWK+;891OpxS$Qk>j50nz`yVp@3?(T
>
>delta 640
>zcmZ9J&1(}u7{=$F-R#aLo5i$EH@gx|NFWI%n@+6Ocrbo|U&W6U3wkP2T6!py+A4wv
>z@#ImL=j2I2FG5fD?#F*%ZuKBkPads^c+=TrRLH>0^YFaCdEbHgRop8+sZ%L#Y+fgE
>zfiY&RtgkcT1J9T#QYvGEgHl!kI7X5rYnWP@`^x`MEz$f}v*T}h@?cS!T-51s&aEXd
>zk5`$kl#zoQ^xa-i)?hXC_@PiX@5*RtDXFqlkvV+e)Dhw_5AlXq4s0!NZZ+4IZ>?w+
>zdwdcnjDzrvO5yyKh0@unDLGT>MD0kH+S9Tl<#Zb_jgvTIT*6Q8;fs;a%{E4e9nXAC
>zG=cXiKNwR3bLve(Pyee8?5pY#m5wpeHO4a9T}IjRX#*XrOcIOM9qd|ldIYllaz!(F
>zt!br>+6CPvfOLFZv+Kb{P0WpuA!6I78N9Mv)Wb)+#fyZrh{9D3J{!4EBg~74KH;J8
>z=?9*QR&ZM*`y(jBXK|H|?MP>l(>uhOxBRQXynBvrA~wgd?N+IPzT0(P=npxHAUjLD
>v=w-Y7w}I2?Njx`AY-g*)(9d@9!4)WY$LSt=Ue`Hg4JSSrw)3hg?|ZdBB6Vuj
>
>diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-report.txt b/tests/data/test-abidiff-exit/test-leaf-peeling-report.txt
>index 7ab4dd6e..48f5df8e 100644
>--- a/tests/data/test-abidiff-exit/test-leaf-peeling-report.txt
>+++ b/tests/data/test-abidiff-exit/test-leaf-peeling-report.txt
>@@ -3,38 +3,38 @@ Changed leaf types summary: 6 leaf types changed
> Removed/Changed/Added functions summary: 0 Removed, 0 Changed, 0 Added function
> Removed/Changed/Added variables summary: 0 Removed, 0 Changed, 0 Added variable
>
>-'struct foo at test-leaf-peeling-v0.cc:1:1' changed:
>+'struct foo at test-leaf-peeling-v0.cc:2:1' changed:
> type size changed from 32 to 64 (in bits)
> there are data member changes:
> type 'int' of 'foo::z' changed:
> type name changed from 'int' to 'long int'
> type size changed from 32 to 64 (in bits)
>
>-'struct ops1 at test-leaf-peeling-v0.cc:5:1' changed:
>+'struct ops1 at test-leaf-peeling-v0.cc:6:1' changed:
> type size hasn't changed
> there are data member changes:
> type 'int*' of 'ops1::x' changed:
> pointer type changed from: 'int*' to: 'int**'
>
>-'struct ops2 at test-leaf-peeling-v0.cc:9:1' changed:
>+'struct ops2 at test-leaf-peeling-v0.cc:10:1' changed:
> type size changed from 320 to 640 (in bits)
> there are data member changes:
> 'foo ops2::y[10]' size changed from 320 to 640 (in bits) (by +320 bits)
>
>-'struct ops3 at test-leaf-peeling-v0.cc:13:1' changed:
>+'struct ops3 at test-leaf-peeling-v0.cc:14:1' changed:
> type size hasn't changed
> there are data member changes:
> type 'void (int&)*' of 'ops3::spong' changed:
> pointer type changed from: 'void (int&)*' to: 'void (int&&)*'
>
>-'struct ops4 at test-leaf-peeling-v0.cc:17:1' changed:
>+'struct ops4 at test-leaf-peeling-v0.cc:18:1' changed:
> type size hasn't changed
> there are data member changes:
> type 'int*' of 'ops4::x' changed:
> entity changed from 'int*' to 'int&'
> type size hasn't changed
>
>-'struct ops5 at test-leaf-peeling-v0.cc:21:1' changed:
>+'struct ops5 at test-leaf-peeling-v0.cc:22:1' changed:
> type size hasn't changed
> there are data member changes:
> type 'int*' of 'ops5::x' changed:
>diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc
>index 745d44f5..2a25b877 100644
>--- a/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc
>+++ b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc
>@@ -1,3 +1,4 @@
>+// See comments in -v1.c file.
> struct foo {
> int z;
> };
>diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v0.o b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.o
>index e6ed6218704e957cbac929f82d0a571fd181103f..cf5fc57b45f185b13b0d61ec9150f25e5c952b69 100644
>GIT binary patch
>delta 559
>zcmZ8dy-Pw-6u;-ar``7+7H0N*wrHkU)YB&oy#^tyglOpxh)A+<un!H&XsDr<Ad~~q
>z64BNYNCu735RK8$(9qP-+|U$6_v)SA8Sd|#-v^v~nz3fAey2-xmJlNE5s(`X!4WTA
>z<>M}aE|qtLc-saKZ<C<WBZhK~fZq6v`?~2p8#S&$52hhL)Jwm(f#m}t-%e8Krvy5J
>zfi;L3VGo~GnP++R@CPE;f;8Sj2!FxEPlltCiG5NC7a7h-q1LHX02sh;If_vl!jzKm
>zRf=n+>UOcRw6R$o$1*jstOOJ`*!ZTL;eld+gXc;%LGroWWF$FVER>elBaW4^>@nLJ
>z?sSn%+IDO^?X)gEQ?l)6+wt;up#4cz^c;dN{u=#i-rNyv*`@B|mTH&@!Ctu3L%d?@
>zgkWD>>M_1D^+d3S*oz;X?^8k0Qm}%yW=J0d=QPXA3Tx42tz&~(Wqi~On8$Z575)*<
>WXP5Jd;Gk}?>o4dA+_Xx1-un+qU{qHC
>
>delta 566
>zcmZ8d&r1SP5Pq}k%I?YrMz*dIteIh2>JuR>3}P#xAn4E`qM~4nJ!GX(*}ZEdL&u2z
>z1HqtU1VPlHh%VJX(6tVog6KWlF8g5kX1*UV^BS>6?D!%8@+2WdJRl&{uR=*lUgP6F
>z0l&gKLi{a*#M=ZY@*zX9MnJ9q#5GO6WTVP8Xu~YTdxP>jH?VxWn{Op$2+0KggifqM
>zOfw`tD<aPle1j<7gMl4F7{3`#FdPsJ+-0~Z7|_++6KVhv{1l_U&8;d$Q#LT^>%%2q
>zDqNbGGp$sL=FLJnm!@VeOAC}{b2uW6;*swpU$NJn>W*DmX3;z$CCeH%6XW)(vr_(x
>z4AP-~Pq=v|jfu3}+K-T+i~LrFRqHB-ArJq7L8WB4B-!w&H@L0nM#^PRJ?b5vGqvcl
>zj~?{_pP2gWvO4=b{{HuI5Y!ATAXRlC4mhQzjd|B%d#o>5XVz)FR&}uORn0_-uH)9@
>U)Bt+|Y3Rn;fDT_xCs6YL0xSAeyZ`_I
>
>diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc
>index 439f7c0b..ea0f835a 100644
>--- a/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc
>+++ b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc
>@@ -1,33 +1,35 @@
>+// Ensure that different pointer / reference types are considered genuinely
>+// different in --leaf-changes-only mode.
> struct foo {
>- long z;
>+ long z; // was int
> };
>
> struct ops1 {
>- int ** x;
>+ int ** x; // was *
> };
>
> struct ops2 {
>- // A change to foo's size is currently considered local here. Arguably this
>- // should be considered non-local as the change to foo is being reported
>- // independently. If this happens, the test case will need to be updated (to
>- // remove the reporting of an ops5 diff).
>+ // A change to foo's size (impacting y's size) is currently considered local
>+ // here. Arguably this should be considered non-local as the change to foo is
>+ // also being reported independently. If this happens, the test case will
>+ // need to be updated (to remove the reporting of the ops2 diff).
> foo y[10];
> };
>
> struct ops3 {
>- void (*spong)(int && wibble);
>+ void (*spong)(int && wibble); // was &
> };
>
> struct ops4 {
>- int & x;
>+ int & x; // was *
> };
>
> struct ops5 {
>- int *** x;
>+ int *** x; // was *
> };
>
> // TODO: This *should* be considered a local change, but currently is not.
>-int var6[5][2];
>+int var6[5][2]; // was [2][5]
>
> void register_ops1(ops1*) { }
> void register_ops2(ops2*) { }
>diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v1.o b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.o
>index 81fcfe0a5a5abecf2d104e71859a04ab529d0b04..c59573e19ddfd8d7b170151a743445ca15be1574 100644
>GIT binary patch
>delta 587
>zcmZ9JF-QVY7{}jtp7ri7Ls2;&HJq7(3F?UiHxxwV5rkW7Em^o!XoLkdI5(&tG_*uS
>z&=5ozG}h46)>czNQ(HrW5Jd0kYq<ya-tYhay*u9HdWl}5b!+%&i4fx3C!n_PqsA~@
>z7qLNrw%_80K|3Pu5eT9I$v~Js@~rcRKm=zXIc3ssv4g)5(*zMRN^5-f3#2ec!!Xd}
>zcm!$F@-v<_pRi&C#RMsADJFgZgJho~3WLc$EpSToX_wPPx1}@z;uu!r=%}ect+G+A
>z?^J55Tif*_uBg*02OB@t)0kUcw&pV#yJ$JtLe{nldE2q=yn|QjEFSw4At&uI-6wy^
>zXOHmni4hts{mW{cA7qYt`g5GqTy`Scvd32OOf%W0Y%e@^6rZ`R%l6J=Ed<?U4cY#9
>z>;c-k32V5hyUGi}j-F*Hx#dt^LHs9i^d`RQIrb~<wZ|4QZsgdPwC^6fgjH^(Bq3!$
>M|6jiB-WcxCAJ3Omv;Y7A
>
>delta 571
>zcmZ9JF-XHe6o&7miChv|thL5-(3G}f71|~gw1c9EEiO(Df`cwqIw`G)Iyg8BrhT{;
>z6r4pWIymT{OE*U+MMoD0hk}cDNp#4A-231E-5r0pZnB$nu63X#LP+R<fYi8;>k8cv
>z;~s&Y#%n@XX;X~*1R|(IIvk~seAf6!pbw`ZJszjuVh2AkpbAAugx2_~2uXp_7<hOH
>zk0E0WD~!)-NKjF|Vu2Jol7Sz<AnB@piQ$Pfhzk;fgzM~bWwssZ2w((bazfeKu3H2P
>z@+h|G7&>woXB7i%Tv2B5TyBMHmCb5>r&3$v8w+@<q@t4<vzWEBmTBj$qGjdnc1J0N
>ztaXuqSJZ3nEfo>NrZYybWy&Z%Yg?z>9?O7KiI*p2UA1ZWJOIlc4U+W<`?Xa;-m
>z-Jbw`LQ6AX6&JO#WCAv|EL-*?9{OFpi-_LA7cIw1zIz#<6PVC*Y|3}v0(2g$oVu^a
>RZ;DD@ioct`+E;qH=ND_rP|*MY
>
>diff --git a/tests/data/test-abidiff-exit/test-leaf-redundant-report.txt b/tests/data/test-abidiff-exit/test-leaf-redundant-report.txt
>index 34b7a848..581fd28e 100644
>--- a/tests/data/test-abidiff-exit/test-leaf-redundant-report.txt
>+++ b/tests/data/test-abidiff-exit/test-leaf-redundant-report.txt
>@@ -5,23 +5,23 @@ Removed/Changed/Added variables summary: 0 Removed, 0 Changed, 0 Added variable
>
> 4 functions with some sub-type change:
>
>- [C] 'function void fn1(sto1)' at test-leaf3-v1.c:10:1 has some sub-type changes:
>+ [C] 'function void fn1(sto1)' at test-leaf-redundant-v1.c:12:1 has some sub-type changes:
> parameter 1 of type 'struct sto1' changed:
> type name changed from 'sto1' to 'stn1'
> type size hasn't changed
>
>- [C] 'function void fn2(sto2)' at test-leaf3-v1.c:13:1 has some sub-type changes:
>+ [C] 'function void fn2(sto2)' at test-leaf-redundant-v1.c:15:1 has some sub-type changes:
> parameter 1 of type 'struct sto2' changed:
> type name changed from 'sto2' to 'stn2'
> type size changed from 64 to 128 (in bits)
> 1 data member insertion:
>- 'double stn2::y', at offset 64 (in bits) at test-leaf3-v1.c:7:1
>+ 'double stn2::y', at offset 64 (in bits) at test-leaf-redundant-v1.c:9:1
>
>- [C] 'function void fn3(sto1*)' at test-leaf3-v1.c:16:1 has some sub-type changes:
>+ [C] 'function void fn3(sto1*)' at test-leaf-redundant-v1.c:18:1 has some sub-type changes:
> parameter 1 of type 'sto1*' changed:
> pointer type changed from: 'sto1*' to: 'stn1*'
>
>- [C] 'function void fn4(sto2*)' at test-leaf3-v1.c:19:1 has some sub-type changes:
>+ [C] 'function void fn4(sto2*)' at test-leaf-redundant-v1.c:21:1 has some sub-type changes:
> parameter 1 of type 'sto2*' changed:
> pointer type changed from: 'sto2*' to: 'stn2*'
>
>diff --git a/tests/data/test-abidiff-exit/test-leaf-redundant-v0.c b/tests/data/test-abidiff-exit/test-leaf-redundant-v0.c
>index 1d9c6266..c5dc8216 100644
>--- a/tests/data/test-abidiff-exit/test-leaf-redundant-v0.c
>+++ b/tests/data/test-abidiff-exit/test-leaf-redundant-v0.c
>@@ -1,3 +1,4 @@
>+// See comments in -v1.c file.
> struct sto1 {
> int x;
> };
>diff --git a/tests/data/test-abidiff-exit/test-leaf-redundant-v0.o b/tests/data/test-abidiff-exit/test-leaf-redundant-v0.o
>index 1e74a51b929f63ba77041febe2b2aa064e41cc78..ef7df991deb2c7337b13d008a2977c0e5fdd37de 100644
>GIT binary patch
>delta 594
>zcmZ`$K~L0B5T0qb?Q3_JP&aH-G_VqbV6scQkgR*z)fh1zAV@;+1QOT+$x=`%di7wu
>zx|xG+xbPE9xSHtQi+l0zKY(yCF(k|^4F?jM&dm4qd*AfEdH?*c!=EScWoPo>k)~z=
>zKz|RYTY>;G8Dl$6KohLcR1sIvtv&)Ulp&HC#WR2fa<MSlWp3>=^8{&~El_Iia?Ld0
>zAg{UY4SK(>wDP4^1<Z@zKW8rfd{JEDx1%@&bZ8en&Y$ql^Ho-KnBt81e<05pQHZ=r
>zYd<NsTWeAIO>5(Ays;K1<%6KI&N}bc2fKe5A-aupsNE|*n_ntD32UW#rCJF~)gY{g
>zVNj<ttufx-jW<g&fvIF?@5a^?=*6M><h2I!i{oXzhyF05Te7?Kand?Mj}6ZsV<zfQ
>z`Zb%@ae8gs9ld@EnMD*=Ii@*g=&xCz@5UWEF_i13PU&xRwCU9FaFcBF9j?#^^9we}
>zHNE4&dZpoS>exRtWznKDi|f>NYWR?PPSgIG4(KEK*&05j7uhDZsl$1XdYl)?%{4WZ
>Wp~YOoKI-2;zys>$YWR(Mx%(I1wQ!*T
>
>delta 575
>zcmZ8eF;Cl25WaI_$9CKz1I0Kk!l)5VhJ-vP2<VVdr3y=_I<yiaw2E6KaDYUCp?k{)
>z+(0|BFc(Fg`vYKMVPfdmvDqpiB<|T!2cF)2clW;U-n+BEW&XJbIh;StcNbA_06=07
>zP)W%MAemIx!wx7i)=||1_LA442td_UNohe?0URb9M<%~>o@#UE$VfT-1iHQ)Ys!E_
>zta<DVIp_bmLd^MqmaKPgIhXa;n5>fZMi2@>gIee?e!;8iTV@h4&4_sO@Bt&DOyuq~
>z+nvHjb1f(rx~1X=KIS%EFrpb`lA%*2Te&aWXoVj=IqOVka|@PUzw_#1&RZz@MdA4+
>zu^>drr{79-8kXwy=U(n*^Zoi-7z+wwE`LwjYb7obZF-?P?jwG5m^(A&>%7vj1DCc{
>z2PbG>of;c7D<=&?pRLSRIZBPvwfe-(4Jzz`mp2(~CfS;UPbshMVunt%W1J<SIlVWj
>zRRw?0nej{cqtTApz!kESBXnVAah&>Q&G<R^9G|1}SLrMY+D_M4cNSrvu|&eEDSs2R
>VV^xi>(T@h0iZ8OVctCyY*&W5DYb*c&
>
>diff --git a/tests/data/test-abidiff-exit/test-leaf-redundant-v1.c b/tests/data/test-abidiff-exit/test-leaf-redundant-v1.c
>index fe182cd8..1763a332 100644
>--- a/tests/data/test-abidiff-exit/test-leaf-redundant-v1.c
>+++ b/tests/data/test-abidiff-exit/test-leaf-redundant-v1.c
>@@ -1,3 +1,5 @@
>+// Ensure that --redundant is indeed implied by --leaf-changes-only mode and
>+// that all the changed functions get complete reports.
> struct stn1 {
> int x;
> };
>@@ -7,14 +9,14 @@ struct stn2 {
> double y;
> };
>
>-void fn1(struct stn1 s) {
>+void fn1(struct stn1 s) { // was sto1
> }
>
>-void fn2(struct stn2 s) {
>+void fn2(struct stn2 s) { // was sto2
> }
>
>-void fn3(struct stn1* s) {
>+void fn3(struct stn1* s) { // was sto1
> }
>
>-void fn4(struct stn2* s) {
>+void fn4(struct stn2* s) { // was sto2
> }
>diff --git a/tests/data/test-abidiff-exit/test-leaf-redundant-v1.o b/tests/data/test-abidiff-exit/test-leaf-redundant-v1.o
>index f7db3a0cd5c0d44e14a1d00e7503a07e50a7e1cd..443777797e4a5fc694cda2cfa46544ab639406c1 100644
>GIT binary patch
>delta 617
>zcmZuuL1+_E5S`i0Cc8;$n@IXiw#Zh3O@&SJBPj9EAQ9umLP1YnlDZoW#%;A*sdx~3
>z@uJZ09IQtVo;>y<dJu13bCF&|ynFB{h~R9p6bXL*e=~pH{5LcFccsJ9UIq8n^6oj^
>zybJ*P4qze`c>sxoF$hA?B7Y83<~_C%Z;n0C9|BtHyNw0)#})<gmT67h2GhLCUa}vY
>z1<X?oZIKsWk2rV2H2`O+hjwmuKFZcq7p#6D0kiZR1>+n1QtL9KLyZyf_pup9#OM<3
>z(BBH(q2KGf_kC|W@OnW=ueB4Hq4(Otd}sB#)0XF*X0=h3PNOcHQr4Sf>nonOvAsU@
>zVYm_8a-#8S;oZ&uskski^(JhE+!R9tpwn(&a4MJl^+7L~2)nYmJXM0*<8|;@A0d`#
>z%@D<te9v>k=`jN!Vtb077y?hzD`PSJ&kP@o1v)e;2!u&d9K^|m*hV52ACF|RQ{0WC
>zt5fV#QvCKt9DOv!UZYRRm$*U~Q{rGV)z$D9jjWG&g;ctO9xW!0(~oo+cPN)>S>MLL
>u5AbXpA7;vUhg7D8FKEQLPq}Oh_h^l=PAc27J{yy2XC^w?GXA98%+g;V0(=?(
>
>delta 613
>zcmZ9IK~EDw6vyA&WwyKB*0ho`Yzt<45L=VB%r??sB0WIp2@_9hyil}NlDa@x48(&d
>zXA?H>z$f6u#SIs4eg(Y{4idsg;6Ml=CeF00hi+#7@BiNK&FtGb&417DmvB2Qf0;pZ
>z5didUKrNvhz{m(&^#jn9&mc25WK`VfJJ5Fk6X8VxN5ek_KFW_I*1~sSnoH6dUB3>P
>zp&Ax=hM)asl>zer)6~X7Ha(`gwQw1%t}g(S^cA_(Lp=HPToN6oq$1wlNlQgkiCRHt
>zGpO`B?QXsDK~!JMG5~YH>QAc@-9m<2%$Z|oK4Ezo_6Dly@}S}En}P2^Z_R(}s$YJy
>zwzbmh*iZ}JuY*Ja=v>R8M_1b16tw2&AG@W6&dO@rcW0|!Ra8WyJaDczEoOz7_Kpwq
>zU+84zAa~Wk@so^Wjy^N)I5JbT7+38$PEy2*Sf}4?g05K^Yg9BixrUj|#U1J#OTPE%
>znbC3{#?_}oG((ZWoqu~i4$*sbVI1HiYA5*deqvd}Q@XT%V2Pq+3t!PqvVkv1*iGvs
>szSBM0u^af3BD;yZblF#tYT`O=NFApr)wI5`!B>iOlWO2k66ptj0k7t5i2wiq
>
>diff --git a/tests/data/test-abidiff-exit/test-leaf-stats-v0.cc b/tests/data/test-abidiff-exit/test-leaf-stats-v0.cc
>index 27ba39c9..12a00e0a 100644
>--- a/tests/data/test-abidiff-exit/test-leaf-stats-v0.cc
>+++ b/tests/data/test-abidiff-exit/test-leaf-stats-v0.cc
>@@ -1,3 +1,4 @@
>+// See comments in -v1.c file.
> int changed_var = 0;
>
> int changed_fun() {
>diff --git a/tests/data/test-abidiff-exit/test-leaf-stats-v0.o b/tests/data/test-abidiff-exit/test-leaf-stats-v0.o
>index a79511cc3c6db039aaa1f5528db19b55f75c7a2d..2faf8314022d140d44519fdc997fa601fc171faf 100644
>GIT binary patch
>delta 396
>zcmY+8K}!N*5Qe|m{nqwdB=BH*h+2zityWzTmhGU0!3IInA*mp2wbhIg(M<>n^b(yy
>znVZn5Um#xcRM-B6-GVM%y|u6iH83+g!^`{5IQSFnDv;<!^T74m^FVvgeUwGY^JD|l
>zu;5dhj^#A*6v9hELE01GnBxe5xqm2U*Vna_Y{Zf=O-?2>L(}jbwk}H-R;^;67g|<>
>z#GUIpaqOm}UfEXJ^HN)hSgAw`Cz>{umTzh;@&^bpv)N5q*|CdNtL8<hni2gMmN15C
>zt^p9j0Uv^8{NN+gZ@>8!AUI3ca7@KOfOsXji@9CF0~&%NR;dYj9MCvC;ir4K#|h2h
>zEA`_Y^$*X4Fb6W;&b)zwhG;?#JE9IpI1)4L0X)k-0@Inyd9;e#Oy^7vBQS?urob+a
>eSO)4iac3J1DZ{>aPnyDxq_aoCqZjP>7JmV|$zwAB
>
>delta 404
>zcmew$`apDo2BX46%`isBi48K0%oFF%Vs~R;U|?oYpKQk{mtvq>W}ugx3>0BtU=?QI
>zWCF8*ga|8_H!EW#6B7d?V{%4fUV3Uud|GK91B_jkSj140T3n)=lbV<YGsWF6RKZzW
>z+t5V8QqM@wP{GK+(9*zi@*GA7QT>wK0*3e~LqnJWK*P!=%QK0JSZJcCHn0TRzS)hb
>zh*44q$PxrP6$V(K47te{nH3ogCjVeoovgs3A*l%!7lhJWK$;VXB_=1bh%?qtu4Hj%
>z<e7YsMW2yj@<*UZ%VbVge@3&(jV$7u3s@zX7zHMGvhQb<nC!{n&geCHB8NGn$K;b7
>z_MDB35EG|P7UVSN6oUqi+T=h^b0!<6$%>3(lNWGmFgi>=$!X7+FqxCfp0Q%GCzn0v
>X1E{0*Cr{)u=d56b7_)ftL^gQ<yL3{)
>
>diff --git a/tests/data/test-abidiff-exit/test-leaf-stats-v1.cc b/tests/data/test-abidiff-exit/test-leaf-stats-v1.cc
>index 020cb761..192d672a 100644
>--- a/tests/data/test-abidiff-exit/test-leaf-stats-v1.cc
>+++ b/tests/data/test-abidiff-exit/test-leaf-stats-v1.cc
>@@ -1,5 +1,7 @@
>-long changed_var = 0;
>+// Test to ensure changed variables and functions are accounted for correctly in
>+// --leaf-changes-only mode.
>+long changed_var = 0; // was int
>
>-long changed_fun() {
>+long changed_fun() { // was int
> return 0;
> }
>diff --git a/tests/data/test-abidiff-exit/test-leaf-stats-v1.o b/tests/data/test-abidiff-exit/test-leaf-stats-v1.o
>index 4a433b9572abb46799391619f21538b54fb3204c..4f2288c96881d79a5fc449679338861170061d08 100644
>GIT binary patch
>delta 477
>zcmZ8eJxjw-6n!^&sYy^lgjN?xF;c&h_7#_k5TQbbf)!Cv7kSoLg&M`CMM1$y#L038
>zi(Byz2wes3=3j8LKfz6K@ugK!5AUAKxg5^>x{TJN*W<8Oo7fnUBUwU-CJX97R+X^R
>zkU*(5Zt*oj6Ec|&HGK5<2?hv7xB{;nSS63|Sml5RW#lkqM-@G%z`7&}XJME7MQ?kv
>z%8!V<xN2mlrr3;;vC}p)(kYf<3|lZ?{1;<YnO!K5WUzloN(X+?*z*Ijxxv_X+)l->
>zl3#)lH=CU`#uvP;J??k&EtZ-1vr{V5%t;l17`EgXjN*$Nk9-~bLmmTyBXkm<s5&qZ
>z-d#W!rHR@?PGfM4C)5?L7EQt#zR}q5O=uEW?zSKQ1;&*;KGWXjsbWfy!bZ;{EFo82
>z$YV_!#0S-cb8M@Qb``#)z+GoPr<s~1btj3=<Ww_Z1sj?JYuFZC$GGmmHFDk2-oO5A
>NrkkmoaF1<0@d?6$Zq)z)
>
>delta 482
>zcmbOs)*&`QgVAH6W|SNwn==Cg11B?(00U+YRuIhsA{dz`E}T{G24pcasDt>7zxjB9
>zBqJk;141RK#U;8qsflR@x@CrX$;m)*1_o9U22Lh03rL8ta(S~dMlvx0^(AK{=B1~m
>z#HW?!F~HbmiA4<hCAkF*Ir(|%3YmE&4DNoR3eMWvh9(M@dPaJN><UH(hL#4Flh-gh
>zc*RE<8p3n}O)10T0C#6+D+LXg)TGSBJctRp7Mc(S)FcB-pu08)GnFz*3ISPyK;OXt
>z2b3W<`606+qrqej7S+iHEE<w(P;o&h%>|@6fw*^aA&WSp#N<vEcSfGc7g_WfWhXPT
>zich}4!XX%hrnY9XA*(;5+2%%8eI`bQ$s5`CGdfHz<Zx&7n!J<4oUvf?M-F?=R#4Ca
>z!Mw?qoaUU$&|p-XT*+z9=s9^Kr#)l9<c~nIW3nL^h%Dr?=llS5zy9Q%T;_}mCV%8o
>F2LLeTULODe
>
>--
>2.26.0.110.g2183baf09c-goog
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Improve readability of represent helper function.
2020-04-08 12:47 ` [PATCH 1/2] Improve readability of represent helper function Matthias Maennich
@ 2020-04-08 14:24 ` Giuliano Procida
2020-04-08 14:57 ` Matthias Maennich
0 siblings, 1 reply; 10+ messages in thread
From: Giuliano Procida @ 2020-04-08 14:24 UTC (permalink / raw)
To: Matthias Maennich; +Cc: libabigail, Dodji Seketeli, kernel-team
Hi.
On Wed, 8 Apr 2020 at 13:47, Matthias Maennich <maennich@google.com> wrote:
>
> On Wed, Apr 08, 2020 at 01:20:43PM +0100, Giuliano Procida wrote:
> >This change:
> >
> > - makes local variables const where possible
> > - gives local variables more descriptive names
> > - factors out some more expressions as local variables
> > - simplifies the logic around anonymous data members
> >
> >There are no behavioural changes.
> >
> > * src/abg-reporter-priv.cc (represent): In the var_diff_sptr
> > overload, rename pretty_representation to
> > o_pretty_representation, introduce n_pretty_representation
> > where needed and replace the duplicate tr1 and tr2 with these;
> > rename all other variables foo1 and foo2 to o_foo and n_foo
> > respectively, using _type instead of _t; introduce o_anon and
> > n_anon and use them to make the local variable
> > is_strict_anonymous_data_member_change const, make ABG_ASSERT
> > in anonymous data member handling more obvious in the case
> > where anonymity has changed and allow simplification of
> > formatting in this case; move declarations of const local
> > variables above those of the non-const, state-tracking,
> > variables.
> >
> >Signed-off-by: Giuliano Procida <gprocida@google.com>
> >---
> > src/abg-reporter-priv.cc | 117 +++++++++++++++++----------------------
> > 1 file changed, 50 insertions(+), 67 deletions(-)
> >
> >diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc
> >index 5fb5fbc5..a46c0e9d 100644
> >--- a/src/abg-reporter-priv.cc
> >+++ b/src/abg-reporter-priv.cc
> >@@ -409,14 +409,21 @@ represent(const var_diff_sptr &diff,
> > if (!ctxt->get_reporter()->diff_to_be_reported(diff.get()))
> > return;
> >
> >- var_decl_sptr o = diff->first_var();
> >- var_decl_sptr n = diff->second_var();
> >- string name1 = o->get_qualified_name();
> >- string name2 = n->get_qualified_name();
> >- uint64_t size1 = get_var_size_in_bits(o);
> >- uint64_t size2 = get_var_size_in_bits(n);
> >- uint64_t offset1 = get_data_member_offset(o);
> >- uint64_t offset2 = get_data_member_offset(n);
> >+ const var_decl_sptr o = diff->first_var();
> >+ const var_decl_sptr n = diff->second_var();
> >+ const bool o_anon = !!is_anonymous_data_member(o);
> >+ const bool n_anon = !!is_anonymous_data_member(n);
> >+ const bool is_strict_anonymous_data_member_change = o_anon && n_anon;
> >+ const string o_name = o->get_qualified_name();
> >+ const string n_name = n->get_qualified_name();
> >+ const uint64_t o_size = get_var_size_in_bits(o);
> >+ const uint64_t n_size = get_var_size_in_bits(n);
> >+ const uint64_t o_offset = get_data_member_offset(o);
> >+ const uint64_t n_offset = get_data_member_offset(n);
> >+ const string o_pretty_representation = o->get_pretty_representation();
>
> Any reason to not have n_pretty_representation also already defined
> here?
That's what I had originally. However, it may be a fairly expensive
operation and it's only needed in relatively rare cases.
Regards,
Giuliano.
> Otherwise nice cleanup!
>
> Reviewed-by: Matthias Maennich <maennich@google.com>
>
> Cheers,
> Matthias
>
> >+
> >+ const bool show_size_offset_changes = ctxt->get_allowed_category()
> >+ & SIZE_OR_OFFSET_CHANGE_CATEGORY;
> >
> > // Has the main diff text been output?
> > bool emitted = false;
> >@@ -424,39 +431,31 @@ represent(const var_diff_sptr &diff,
> > bool begin_with_and = false;
> > // Have we reported a size change already?
> > bool size_reported = false;
> >- // Are we reporting a change to an anonymous member?
> >- bool is_strict_anonymous_data_member_change = false;
> >
> >- string pretty_representation = o->get_pretty_representation();
> >- bool show_size_offset_changes = ctxt->get_allowed_category()
> >- & SIZE_OR_OFFSET_CHANGE_CATEGORY;
> >-
> >- if (is_anonymous_data_member(o) && is_anonymous_data_member(n))
> >+ if (is_strict_anonymous_data_member_change)
> > {
> >- is_strict_anonymous_data_member_change = true;
> >- string tr1 = o->get_pretty_representation();
> >- string tr2 = n->get_pretty_representation();
> >- type_base_sptr t1 = o->get_type(), t2 = n->get_type();
> >- if (tr1 != tr2)
> >+ const string n_pretty_representation = n->get_pretty_representation();
> >+ const type_base_sptr o_type = o->get_type(), n_type = n->get_type();
> >+ if (o_pretty_representation != n_pretty_representation)
> > {
> > show_offset_or_size(indent + "anonymous data member at offset",
> >- offset1, *ctxt, out);
> >+ o_offset, *ctxt, out);
> >
> > out << " changed from:\n"
> >- << indent << " " << tr1 << "\n"
> >+ << indent << " " << o_pretty_representation << "\n"
> > << indent << "to:\n"
> >- << indent << " " << tr2 << "\n";
> >+ << indent << " " << n_pretty_representation << "\n";
> >
> > begin_with_and = true;
> > emitted = true;
> > }
> >- else if (get_type_name(t1) != get_type_name(t2)
> >- && is_decl(t1) && is_decl(t2)
> >- && is_decl(t1)->get_is_anonymous()
> >- && is_decl(t2)->get_is_anonymous())
> >+ else if (get_type_name(o_type) != get_type_name(n_type)
> >+ && is_decl(o_type) && is_decl(n_type)
> >+ && is_decl(o_type)->get_is_anonymous()
> >+ && is_decl(n_type)->get_is_anonymous())
> > {
> > out << indent << "while looking at anonymous data member '"
> >- << tr1 << "':\n"
> >+ << o_pretty_representation << "':\n"
> > << indent << "the internal name of that anonymous data member"
> > "changed from:\n"
> > << indent << " " << get_type_name(o->get_type()) << "\n"
> >@@ -472,36 +471,20 @@ represent(const var_diff_sptr &diff,
> > }
> > else if (filtering::has_anonymous_data_member_change(diff))
> > {
> >+ ABG_ASSERT(o_anon != n_anon);
> > // So we are looking at a non-anonymous data member change from
> > // or to anonymous data member.
> >- if (is_anonymous_data_member(o))
> >- {
> >- string repr = "anonymous data member "
> >- + o->get_pretty_representation()
> >- + " at offset";
> >-
> >- show_offset_or_size(indent + repr, offset1, *ctxt, out);
> >- out << " became data member '"
> >- << n->get_pretty_representation()
> >- << "'\n";
> >- }
> >- else
> >- {
> >- ABG_ASSERT(is_anonymous_data_member(n));
> >- string repr = "data member "
> >- + o->get_pretty_representation()
> >- + " at offset";
> >-
> >- show_offset_or_size(indent + repr, offset1, *ctxt, out);
> >- out << " became anonymous data member '"
> >- << n->get_pretty_representation()
> >- << "'\n";
> >- }
> >+ const string n_pretty_representation = n->get_pretty_representation();
> >+ out << indent << (o_anon ? "anonymous " : "")
> >+ << "data member " << o_pretty_representation;
> >+ show_offset_or_size(" at offset", o_offset, *ctxt, out);
> >+ out << " became " << (n_anon ? "anonymous " : "")
> >+ << "data member '" << n_pretty_representation << "'\n";
> >
> > begin_with_and = true;
> > emitted = true;
> > }
> >- else if (diff_sptr d = diff->type_diff())
> >+ else if (const diff_sptr d = diff->type_diff())
> > {
> > if (ctxt->get_reporter()->diff_to_be_reported(d.get()))
> > {
> >@@ -511,7 +494,7 @@ represent(const var_diff_sptr &diff,
> > << "' changed";
> > else
> > out << indent
> >- << "type of '" << pretty_representation << "' changed";
> >+ << "type of '" << o_pretty_representation << "' changed";
> >
> > if (d->currently_reporting())
> > out << ", as being reported\n";
> >@@ -529,7 +512,7 @@ represent(const var_diff_sptr &diff,
> > }
> > }
> >
> >- if (!filtering::has_anonymous_data_member_change(diff) && name1 != name2)
> >+ if (!filtering::has_anonymous_data_member_change(diff) && o_name != n_name)
> > {
> > if (filtering::has_harmless_name_change(o, n)
> > && !(ctxt->get_allowed_category()
> >@@ -546,7 +529,7 @@ represent(const var_diff_sptr &diff,
> > out << indent;
> > else
> > out << ", ";
> >- out << "name of '" << name1 << "' changed to '" << name2 << "'";
> >+ out << "name of '" << o_name << "' changed to '" << n_name << "'";
> > report_loc_info(n, *ctxt, out);
> > emitted = true;
> > }
> >@@ -561,7 +544,7 @@ represent(const var_diff_sptr &diff,
> > begin_with_and = false;
> > }
> > else if (!emitted)
> >- out << indent << "'" << pretty_representation << "' ";
> >+ out << indent << "'" << o_pretty_representation << "' ";
> > else
> > out << ", ";
> > if (get_data_member_is_laid_out(o))
> >@@ -572,7 +555,7 @@ represent(const var_diff_sptr &diff,
> > }
> > if (show_size_offset_changes)
> > {
> >- if (offset1 != offset2)
> >+ if (o_offset != n_offset)
> > {
> > if (begin_with_and)
> > {
> >@@ -584,17 +567,17 @@ represent(const var_diff_sptr &diff,
> > out << indent;
> > if (is_strict_anonymous_data_member_change)
> > out << "anonymous data member ";
> >- out << "'" << pretty_representation << "' ";
> >+ out << "'" << o_pretty_representation << "' ";
> > }
> > else
> > out << ", ";
> >
> >- show_numerical_change("offset", offset1, offset2, *ctxt, out);
> >+ show_numerical_change("offset", o_offset, n_offset, *ctxt, out);
> > maybe_show_relative_offset_change(diff, *ctxt, out);
> > emitted = true;
> > }
> >
> >- if (!size_reported && size1 != size2)
> >+ if (!size_reported && o_size != n_size)
> > {
> > if (begin_with_and)
> > {
> >@@ -606,12 +589,12 @@ represent(const var_diff_sptr &diff,
> > out << indent;
> > if (is_strict_anonymous_data_member_change)
> > out << "anonymous data member ";
> >- out << "'" << pretty_representation << "' ";
> >+ out << "'" << o_pretty_representation << "' ";
> > }
> > else
> > out << ", ";
> >
> >- show_numerical_change("size", size1, size2, *ctxt, out);
> >+ show_numerical_change("size", o_size, n_size, *ctxt, out);
> > maybe_show_relative_size_change(diff, *ctxt, out);
> > emitted = true;
> > }
> >@@ -624,7 +607,7 @@ represent(const var_diff_sptr &diff,
> > begin_with_and = false;
> > }
> > else if (!emitted)
> >- out << indent << "'" << pretty_representation << "' ";
> >+ out << indent << "'" << o_pretty_representation << "' ";
> > else
> > out << ", ";
> > out << "elf binding changed from " << o->get_binding()
> >@@ -639,7 +622,7 @@ represent(const var_diff_sptr &diff,
> > begin_with_and = false;
> > }
> > else if (!emitted)
> >- out << indent << "'" << pretty_representation << "' ";
> >+ out << indent << "'" << o_pretty_representation << "' ";
> > else
> > out << ", ";
> > out << "visibility changed from " << o->get_visibility()
> >@@ -656,7 +639,7 @@ represent(const var_diff_sptr &diff,
> > begin_with_and = false;
> > }
> > else if (!emitted)
> >- out << indent << "'" << pretty_representation << "' ";
> >+ out << indent << "'" << o_pretty_representation << "' ";
> > else
> > out << ", ";
> >
> >@@ -675,7 +658,7 @@ represent(const var_diff_sptr &diff,
> > begin_with_and = false;
> > }
> > else if (!emitted)
> >- out << indent << "'" << pretty_representation << "' ";
> >+ out << indent << "'" << o_pretty_representation << "' ";
> > else
> > out << ", ";
> >
> >@@ -691,7 +674,7 @@ represent(const var_diff_sptr &diff,
> > ;
> > else if (!emitted)
> > // We appear to have fallen off the edge of the map.
> >- out << indent << "'" << pretty_representation
> >+ out << indent << "'" << o_pretty_representation
> > << "' has *some* difference - please report as a bug";
> > else
> > // do nothing
> >--
> >2.26.0.110.g2183baf09c-goog
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Improve readability of represent helper function.
2020-04-08 14:24 ` Giuliano Procida
@ 2020-04-08 14:57 ` Matthias Maennich
2020-04-08 16:38 ` Giuliano Procida
0 siblings, 1 reply; 10+ messages in thread
From: Matthias Maennich @ 2020-04-08 14:57 UTC (permalink / raw)
To: Giuliano Procida; +Cc: libabigail, Dodji Seketeli, kernel-team
On Wed, Apr 08, 2020 at 03:24:24PM +0100, Giuliano Procida wrote:
>Hi.
>
>On Wed, 8 Apr 2020 at 13:47, Matthias Maennich <maennich@google.com> wrote:
>>
>> On Wed, Apr 08, 2020 at 01:20:43PM +0100, Giuliano Procida wrote:
>> >This change:
>> >
>> > - makes local variables const where possible
>> > - gives local variables more descriptive names
>> > - factors out some more expressions as local variables
>> > - simplifies the logic around anonymous data members
>> >
>> >There are no behavioural changes.
>> >
>> > * src/abg-reporter-priv.cc (represent): In the var_diff_sptr
>> > overload, rename pretty_representation to
>> > o_pretty_representation, introduce n_pretty_representation
>> > where needed and replace the duplicate tr1 and tr2 with these;
>> > rename all other variables foo1 and foo2 to o_foo and n_foo
>> > respectively, using _type instead of _t; introduce o_anon and
>> > n_anon and use them to make the local variable
>> > is_strict_anonymous_data_member_change const, make ABG_ASSERT
>> > in anonymous data member handling more obvious in the case
>> > where anonymity has changed and allow simplification of
>> > formatting in this case; move declarations of const local
>> > variables above those of the non-const, state-tracking,
>> > variables.
>> >
>> >Signed-off-by: Giuliano Procida <gprocida@google.com>
>> >---
>> > src/abg-reporter-priv.cc | 117 +++++++++++++++++----------------------
>> > 1 file changed, 50 insertions(+), 67 deletions(-)
>> >
>> >diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc
>> >index 5fb5fbc5..a46c0e9d 100644
>> >--- a/src/abg-reporter-priv.cc
>> >+++ b/src/abg-reporter-priv.cc
>> >@@ -409,14 +409,21 @@ represent(const var_diff_sptr &diff,
>> > if (!ctxt->get_reporter()->diff_to_be_reported(diff.get()))
>> > return;
>> >
>> >- var_decl_sptr o = diff->first_var();
>> >- var_decl_sptr n = diff->second_var();
>> >- string name1 = o->get_qualified_name();
>> >- string name2 = n->get_qualified_name();
>> >- uint64_t size1 = get_var_size_in_bits(o);
>> >- uint64_t size2 = get_var_size_in_bits(n);
>> >- uint64_t offset1 = get_data_member_offset(o);
>> >- uint64_t offset2 = get_data_member_offset(n);
>> >+ const var_decl_sptr o = diff->first_var();
>> >+ const var_decl_sptr n = diff->second_var();
>> >+ const bool o_anon = !!is_anonymous_data_member(o);
>> >+ const bool n_anon = !!is_anonymous_data_member(n);
>> >+ const bool is_strict_anonymous_data_member_change = o_anon && n_anon;
>> >+ const string o_name = o->get_qualified_name();
>> >+ const string n_name = n->get_qualified_name();
>> >+ const uint64_t o_size = get_var_size_in_bits(o);
>> >+ const uint64_t n_size = get_var_size_in_bits(n);
>> >+ const uint64_t o_offset = get_data_member_offset(o);
>> >+ const uint64_t n_offset = get_data_member_offset(n);
>> >+ const string o_pretty_representation = o->get_pretty_representation();
>>
>> Any reason to not have n_pretty_representation also already defined
>> here?
>
>That's what I had originally. However, it may be a fairly expensive
>operation and it's only needed in relatively rare cases.
Fair enough. Maybe add a comment then.
Cheers,
Matthias
>
>Regards,
>Giuliano.
>
>> Otherwise nice cleanup!
>>
>> Reviewed-by: Matthias Maennich <maennich@google.com>
>>
>> Cheers,
>> Matthias
>>
>> >+
>> >+ const bool show_size_offset_changes = ctxt->get_allowed_category()
>> >+ & SIZE_OR_OFFSET_CHANGE_CATEGORY;
>> >
>> > // Has the main diff text been output?
>> > bool emitted = false;
>> >@@ -424,39 +431,31 @@ represent(const var_diff_sptr &diff,
>> > bool begin_with_and = false;
>> > // Have we reported a size change already?
>> > bool size_reported = false;
>> >- // Are we reporting a change to an anonymous member?
>> >- bool is_strict_anonymous_data_member_change = false;
>> >
>> >- string pretty_representation = o->get_pretty_representation();
>> >- bool show_size_offset_changes = ctxt->get_allowed_category()
>> >- & SIZE_OR_OFFSET_CHANGE_CATEGORY;
>> >-
>> >- if (is_anonymous_data_member(o) && is_anonymous_data_member(n))
>> >+ if (is_strict_anonymous_data_member_change)
>> > {
>> >- is_strict_anonymous_data_member_change = true;
>> >- string tr1 = o->get_pretty_representation();
>> >- string tr2 = n->get_pretty_representation();
>> >- type_base_sptr t1 = o->get_type(), t2 = n->get_type();
>> >- if (tr1 != tr2)
>> >+ const string n_pretty_representation = n->get_pretty_representation();
>> >+ const type_base_sptr o_type = o->get_type(), n_type = n->get_type();
>> >+ if (o_pretty_representation != n_pretty_representation)
>> > {
>> > show_offset_or_size(indent + "anonymous data member at offset",
>> >- offset1, *ctxt, out);
>> >+ o_offset, *ctxt, out);
>> >
>> > out << " changed from:\n"
>> >- << indent << " " << tr1 << "\n"
>> >+ << indent << " " << o_pretty_representation << "\n"
>> > << indent << "to:\n"
>> >- << indent << " " << tr2 << "\n";
>> >+ << indent << " " << n_pretty_representation << "\n";
>> >
>> > begin_with_and = true;
>> > emitted = true;
>> > }
>> >- else if (get_type_name(t1) != get_type_name(t2)
>> >- && is_decl(t1) && is_decl(t2)
>> >- && is_decl(t1)->get_is_anonymous()
>> >- && is_decl(t2)->get_is_anonymous())
>> >+ else if (get_type_name(o_type) != get_type_name(n_type)
>> >+ && is_decl(o_type) && is_decl(n_type)
>> >+ && is_decl(o_type)->get_is_anonymous()
>> >+ && is_decl(n_type)->get_is_anonymous())
>> > {
>> > out << indent << "while looking at anonymous data member '"
>> >- << tr1 << "':\n"
>> >+ << o_pretty_representation << "':\n"
>> > << indent << "the internal name of that anonymous data member"
>> > "changed from:\n"
>> > << indent << " " << get_type_name(o->get_type()) << "\n"
>> >@@ -472,36 +471,20 @@ represent(const var_diff_sptr &diff,
>> > }
>> > else if (filtering::has_anonymous_data_member_change(diff))
>> > {
>> >+ ABG_ASSERT(o_anon != n_anon);
>> > // So we are looking at a non-anonymous data member change from
>> > // or to anonymous data member.
>> >- if (is_anonymous_data_member(o))
>> >- {
>> >- string repr = "anonymous data member "
>> >- + o->get_pretty_representation()
>> >- + " at offset";
>> >-
>> >- show_offset_or_size(indent + repr, offset1, *ctxt, out);
>> >- out << " became data member '"
>> >- << n->get_pretty_representation()
>> >- << "'\n";
>> >- }
>> >- else
>> >- {
>> >- ABG_ASSERT(is_anonymous_data_member(n));
>> >- string repr = "data member "
>> >- + o->get_pretty_representation()
>> >- + " at offset";
>> >-
>> >- show_offset_or_size(indent + repr, offset1, *ctxt, out);
>> >- out << " became anonymous data member '"
>> >- << n->get_pretty_representation()
>> >- << "'\n";
>> >- }
>> >+ const string n_pretty_representation = n->get_pretty_representation();
>> >+ out << indent << (o_anon ? "anonymous " : "")
>> >+ << "data member " << o_pretty_representation;
>> >+ show_offset_or_size(" at offset", o_offset, *ctxt, out);
>> >+ out << " became " << (n_anon ? "anonymous " : "")
>> >+ << "data member '" << n_pretty_representation << "'\n";
>> >
>> > begin_with_and = true;
>> > emitted = true;
>> > }
>> >- else if (diff_sptr d = diff->type_diff())
>> >+ else if (const diff_sptr d = diff->type_diff())
>> > {
>> > if (ctxt->get_reporter()->diff_to_be_reported(d.get()))
>> > {
>> >@@ -511,7 +494,7 @@ represent(const var_diff_sptr &diff,
>> > << "' changed";
>> > else
>> > out << indent
>> >- << "type of '" << pretty_representation << "' changed";
>> >+ << "type of '" << o_pretty_representation << "' changed";
>> >
>> > if (d->currently_reporting())
>> > out << ", as being reported\n";
>> >@@ -529,7 +512,7 @@ represent(const var_diff_sptr &diff,
>> > }
>> > }
>> >
>> >- if (!filtering::has_anonymous_data_member_change(diff) && name1 != name2)
>> >+ if (!filtering::has_anonymous_data_member_change(diff) && o_name != n_name)
>> > {
>> > if (filtering::has_harmless_name_change(o, n)
>> > && !(ctxt->get_allowed_category()
>> >@@ -546,7 +529,7 @@ represent(const var_diff_sptr &diff,
>> > out << indent;
>> > else
>> > out << ", ";
>> >- out << "name of '" << name1 << "' changed to '" << name2 << "'";
>> >+ out << "name of '" << o_name << "' changed to '" << n_name << "'";
>> > report_loc_info(n, *ctxt, out);
>> > emitted = true;
>> > }
>> >@@ -561,7 +544,7 @@ represent(const var_diff_sptr &diff,
>> > begin_with_and = false;
>> > }
>> > else if (!emitted)
>> >- out << indent << "'" << pretty_representation << "' ";
>> >+ out << indent << "'" << o_pretty_representation << "' ";
>> > else
>> > out << ", ";
>> > if (get_data_member_is_laid_out(o))
>> >@@ -572,7 +555,7 @@ represent(const var_diff_sptr &diff,
>> > }
>> > if (show_size_offset_changes)
>> > {
>> >- if (offset1 != offset2)
>> >+ if (o_offset != n_offset)
>> > {
>> > if (begin_with_and)
>> > {
>> >@@ -584,17 +567,17 @@ represent(const var_diff_sptr &diff,
>> > out << indent;
>> > if (is_strict_anonymous_data_member_change)
>> > out << "anonymous data member ";
>> >- out << "'" << pretty_representation << "' ";
>> >+ out << "'" << o_pretty_representation << "' ";
>> > }
>> > else
>> > out << ", ";
>> >
>> >- show_numerical_change("offset", offset1, offset2, *ctxt, out);
>> >+ show_numerical_change("offset", o_offset, n_offset, *ctxt, out);
>> > maybe_show_relative_offset_change(diff, *ctxt, out);
>> > emitted = true;
>> > }
>> >
>> >- if (!size_reported && size1 != size2)
>> >+ if (!size_reported && o_size != n_size)
>> > {
>> > if (begin_with_and)
>> > {
>> >@@ -606,12 +589,12 @@ represent(const var_diff_sptr &diff,
>> > out << indent;
>> > if (is_strict_anonymous_data_member_change)
>> > out << "anonymous data member ";
>> >- out << "'" << pretty_representation << "' ";
>> >+ out << "'" << o_pretty_representation << "' ";
>> > }
>> > else
>> > out << ", ";
>> >
>> >- show_numerical_change("size", size1, size2, *ctxt, out);
>> >+ show_numerical_change("size", o_size, n_size, *ctxt, out);
>> > maybe_show_relative_size_change(diff, *ctxt, out);
>> > emitted = true;
>> > }
>> >@@ -624,7 +607,7 @@ represent(const var_diff_sptr &diff,
>> > begin_with_and = false;
>> > }
>> > else if (!emitted)
>> >- out << indent << "'" << pretty_representation << "' ";
>> >+ out << indent << "'" << o_pretty_representation << "' ";
>> > else
>> > out << ", ";
>> > out << "elf binding changed from " << o->get_binding()
>> >@@ -639,7 +622,7 @@ represent(const var_diff_sptr &diff,
>> > begin_with_and = false;
>> > }
>> > else if (!emitted)
>> >- out << indent << "'" << pretty_representation << "' ";
>> >+ out << indent << "'" << o_pretty_representation << "' ";
>> > else
>> > out << ", ";
>> > out << "visibility changed from " << o->get_visibility()
>> >@@ -656,7 +639,7 @@ represent(const var_diff_sptr &diff,
>> > begin_with_and = false;
>> > }
>> > else if (!emitted)
>> >- out << indent << "'" << pretty_representation << "' ";
>> >+ out << indent << "'" << o_pretty_representation << "' ";
>> > else
>> > out << ", ";
>> >
>> >@@ -675,7 +658,7 @@ represent(const var_diff_sptr &diff,
>> > begin_with_and = false;
>> > }
>> > else if (!emitted)
>> >- out << indent << "'" << pretty_representation << "' ";
>> >+ out << indent << "'" << o_pretty_representation << "' ";
>> > else
>> > out << ", ";
>> >
>> >@@ -691,7 +674,7 @@ represent(const var_diff_sptr &diff,
>> > ;
>> > else if (!emitted)
>> > // We appear to have fallen off the edge of the map.
>> >- out << indent << "'" << pretty_representation
>> >+ out << indent << "'" << o_pretty_representation
>> > << "' has *some* difference - please report as a bug";
>> > else
>> > // do nothing
>> >--
>> >2.26.0.110.g2183baf09c-goog
>> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Improve readability of represent helper function.
2020-04-08 14:57 ` Matthias Maennich
@ 2020-04-08 16:38 ` Giuliano Procida
2020-04-08 16:40 ` [PATCH v2 " Giuliano Procida
0 siblings, 1 reply; 10+ messages in thread
From: Giuliano Procida @ 2020-04-08 16:38 UTC (permalink / raw)
To: Matthias Maennich; +Cc: libabigail, Dodji Seketeli, kernel-team
On Wed, 8 Apr 2020 at 16:29, Matthias Maennich <maennich@google.com> wrote:
>
> On Wed, Apr 08, 2020 at 03:24:24PM +0100, Giuliano Procida wrote:
> >Hi.
> >
> >On Wed, 8 Apr 2020 at 13:47, Matthias Maennich <maennich@google.com> wrote:
> >>
> >> On Wed, Apr 08, 2020 at 01:20:43PM +0100, Giuliano Procida wrote:
> >> >This change:
> >> >
> >> > - makes local variables const where possible
> >> > - gives local variables more descriptive names
> >> > - factors out some more expressions as local variables
> >> > - simplifies the logic around anonymous data members
> >> >
> >> >There are no behavioural changes.
> >> >
> >> > * src/abg-reporter-priv.cc (represent): In the var_diff_sptr
> >> > overload, rename pretty_representation to
> >> > o_pretty_representation, introduce n_pretty_representation
> >> > where needed and replace the duplicate tr1 and tr2 with these;
> >> > rename all other variables foo1 and foo2 to o_foo and n_foo
> >> > respectively, using _type instead of _t; introduce o_anon and
> >> > n_anon and use them to make the local variable
> >> > is_strict_anonymous_data_member_change const, make ABG_ASSERT
> >> > in anonymous data member handling more obvious in the case
> >> > where anonymity has changed and allow simplification of
> >> > formatting in this case; move declarations of const local
> >> > variables above those of the non-const, state-tracking,
> >> > variables.
> >> >
> >> >Signed-off-by: Giuliano Procida <gprocida@google.com>
> >> >---
> >> > src/abg-reporter-priv.cc | 117 +++++++++++++++++----------------------
> >> > 1 file changed, 50 insertions(+), 67 deletions(-)
> >> >
> >> >diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc
> >> >index 5fb5fbc5..a46c0e9d 100644
> >> >--- a/src/abg-reporter-priv.cc
> >> >+++ b/src/abg-reporter-priv.cc
> >> >@@ -409,14 +409,21 @@ represent(const var_diff_sptr &diff,
> >> > if (!ctxt->get_reporter()->diff_to_be_reported(diff.get()))
> >> > return;
> >> >
> >> >- var_decl_sptr o = diff->first_var();
> >> >- var_decl_sptr n = diff->second_var();
> >> >- string name1 = o->get_qualified_name();
> >> >- string name2 = n->get_qualified_name();
> >> >- uint64_t size1 = get_var_size_in_bits(o);
> >> >- uint64_t size2 = get_var_size_in_bits(n);
> >> >- uint64_t offset1 = get_data_member_offset(o);
> >> >- uint64_t offset2 = get_data_member_offset(n);
> >> >+ const var_decl_sptr o = diff->first_var();
> >> >+ const var_decl_sptr n = diff->second_var();
> >> >+ const bool o_anon = !!is_anonymous_data_member(o);
> >> >+ const bool n_anon = !!is_anonymous_data_member(n);
> >> >+ const bool is_strict_anonymous_data_member_change = o_anon && n_anon;
> >> >+ const string o_name = o->get_qualified_name();
> >> >+ const string n_name = n->get_qualified_name();
> >> >+ const uint64_t o_size = get_var_size_in_bits(o);
> >> >+ const uint64_t n_size = get_var_size_in_bits(n);
> >> >+ const uint64_t o_offset = get_data_member_offset(o);
> >> >+ const uint64_t n_offset = get_data_member_offset(n);
> >> >+ const string o_pretty_representation = o->get_pretty_representation();
> >>
> >> Any reason to not have n_pretty_representation also already defined
> >> here?
> >
> >That's what I had originally. However, it may be a fairly expensive
> >operation and it's only needed in relatively rare cases.
>
> Fair enough. Maybe add a comment then.
Will do. Also spotted a missed subexpression elimination. Revision
patch on the way.
Giuliano.
> Cheers,
> Matthias
>
> >
> >Regards,
> >Giuliano.
> >
> >> Otherwise nice cleanup!
> >>
> >> Reviewed-by: Matthias Maennich <maennich@google.com>
> >>
> >> Cheers,
> >> Matthias
> >>
> >> >+
> >> >+ const bool show_size_offset_changes = ctxt->get_allowed_category()
> >> >+ & SIZE_OR_OFFSET_CHANGE_CATEGORY;
> >> >
> >> > // Has the main diff text been output?
> >> > bool emitted = false;
> >> >@@ -424,39 +431,31 @@ represent(const var_diff_sptr &diff,
> >> > bool begin_with_and = false;
> >> > // Have we reported a size change already?
> >> > bool size_reported = false;
> >> >- // Are we reporting a change to an anonymous member?
> >> >- bool is_strict_anonymous_data_member_change = false;
> >> >
> >> >- string pretty_representation = o->get_pretty_representation();
> >> >- bool show_size_offset_changes = ctxt->get_allowed_category()
> >> >- & SIZE_OR_OFFSET_CHANGE_CATEGORY;
> >> >-
> >> >- if (is_anonymous_data_member(o) && is_anonymous_data_member(n))
> >> >+ if (is_strict_anonymous_data_member_change)
> >> > {
> >> >- is_strict_anonymous_data_member_change = true;
> >> >- string tr1 = o->get_pretty_representation();
> >> >- string tr2 = n->get_pretty_representation();
> >> >- type_base_sptr t1 = o->get_type(), t2 = n->get_type();
> >> >- if (tr1 != tr2)
> >> >+ const string n_pretty_representation = n->get_pretty_representation();
> >> >+ const type_base_sptr o_type = o->get_type(), n_type = n->get_type();
> >> >+ if (o_pretty_representation != n_pretty_representation)
> >> > {
> >> > show_offset_or_size(indent + "anonymous data member at offset",
> >> >- offset1, *ctxt, out);
> >> >+ o_offset, *ctxt, out);
> >> >
> >> > out << " changed from:\n"
> >> >- << indent << " " << tr1 << "\n"
> >> >+ << indent << " " << o_pretty_representation << "\n"
> >> > << indent << "to:\n"
> >> >- << indent << " " << tr2 << "\n";
> >> >+ << indent << " " << n_pretty_representation << "\n";
> >> >
> >> > begin_with_and = true;
> >> > emitted = true;
> >> > }
> >> >- else if (get_type_name(t1) != get_type_name(t2)
> >> >- && is_decl(t1) && is_decl(t2)
> >> >- && is_decl(t1)->get_is_anonymous()
> >> >- && is_decl(t2)->get_is_anonymous())
> >> >+ else if (get_type_name(o_type) != get_type_name(n_type)
> >> >+ && is_decl(o_type) && is_decl(n_type)
> >> >+ && is_decl(o_type)->get_is_anonymous()
> >> >+ && is_decl(n_type)->get_is_anonymous())
> >> > {
> >> > out << indent << "while looking at anonymous data member '"
> >> >- << tr1 << "':\n"
> >> >+ << o_pretty_representation << "':\n"
> >> > << indent << "the internal name of that anonymous data member"
> >> > "changed from:\n"
> >> > << indent << " " << get_type_name(o->get_type()) << "\n"
> >> >@@ -472,36 +471,20 @@ represent(const var_diff_sptr &diff,
> >> > }
> >> > else if (filtering::has_anonymous_data_member_change(diff))
> >> > {
> >> >+ ABG_ASSERT(o_anon != n_anon);
> >> > // So we are looking at a non-anonymous data member change from
> >> > // or to anonymous data member.
> >> >- if (is_anonymous_data_member(o))
> >> >- {
> >> >- string repr = "anonymous data member "
> >> >- + o->get_pretty_representation()
> >> >- + " at offset";
> >> >-
> >> >- show_offset_or_size(indent + repr, offset1, *ctxt, out);
> >> >- out << " became data member '"
> >> >- << n->get_pretty_representation()
> >> >- << "'\n";
> >> >- }
> >> >- else
> >> >- {
> >> >- ABG_ASSERT(is_anonymous_data_member(n));
> >> >- string repr = "data member "
> >> >- + o->get_pretty_representation()
> >> >- + " at offset";
> >> >-
> >> >- show_offset_or_size(indent + repr, offset1, *ctxt, out);
> >> >- out << " became anonymous data member '"
> >> >- << n->get_pretty_representation()
> >> >- << "'\n";
> >> >- }
> >> >+ const string n_pretty_representation = n->get_pretty_representation();
> >> >+ out << indent << (o_anon ? "anonymous " : "")
> >> >+ << "data member " << o_pretty_representation;
> >> >+ show_offset_or_size(" at offset", o_offset, *ctxt, out);
> >> >+ out << " became " << (n_anon ? "anonymous " : "")
> >> >+ << "data member '" << n_pretty_representation << "'\n";
> >> >
> >> > begin_with_and = true;
> >> > emitted = true;
> >> > }
> >> >- else if (diff_sptr d = diff->type_diff())
> >> >+ else if (const diff_sptr d = diff->type_diff())
> >> > {
> >> > if (ctxt->get_reporter()->diff_to_be_reported(d.get()))
> >> > {
> >> >@@ -511,7 +494,7 @@ represent(const var_diff_sptr &diff,
> >> > << "' changed";
> >> > else
> >> > out << indent
> >> >- << "type of '" << pretty_representation << "' changed";
> >> >+ << "type of '" << o_pretty_representation << "' changed";
> >> >
> >> > if (d->currently_reporting())
> >> > out << ", as being reported\n";
> >> >@@ -529,7 +512,7 @@ represent(const var_diff_sptr &diff,
> >> > }
> >> > }
> >> >
> >> >- if (!filtering::has_anonymous_data_member_change(diff) && name1 != name2)
> >> >+ if (!filtering::has_anonymous_data_member_change(diff) && o_name != n_name)
> >> > {
> >> > if (filtering::has_harmless_name_change(o, n)
> >> > && !(ctxt->get_allowed_category()
> >> >@@ -546,7 +529,7 @@ represent(const var_diff_sptr &diff,
> >> > out << indent;
> >> > else
> >> > out << ", ";
> >> >- out << "name of '" << name1 << "' changed to '" << name2 << "'";
> >> >+ out << "name of '" << o_name << "' changed to '" << n_name << "'";
> >> > report_loc_info(n, *ctxt, out);
> >> > emitted = true;
> >> > }
> >> >@@ -561,7 +544,7 @@ represent(const var_diff_sptr &diff,
> >> > begin_with_and = false;
> >> > }
> >> > else if (!emitted)
> >> >- out << indent << "'" << pretty_representation << "' ";
> >> >+ out << indent << "'" << o_pretty_representation << "' ";
> >> > else
> >> > out << ", ";
> >> > if (get_data_member_is_laid_out(o))
> >> >@@ -572,7 +555,7 @@ represent(const var_diff_sptr &diff,
> >> > }
> >> > if (show_size_offset_changes)
> >> > {
> >> >- if (offset1 != offset2)
> >> >+ if (o_offset != n_offset)
> >> > {
> >> > if (begin_with_and)
> >> > {
> >> >@@ -584,17 +567,17 @@ represent(const var_diff_sptr &diff,
> >> > out << indent;
> >> > if (is_strict_anonymous_data_member_change)
> >> > out << "anonymous data member ";
> >> >- out << "'" << pretty_representation << "' ";
> >> >+ out << "'" << o_pretty_representation << "' ";
> >> > }
> >> > else
> >> > out << ", ";
> >> >
> >> >- show_numerical_change("offset", offset1, offset2, *ctxt, out);
> >> >+ show_numerical_change("offset", o_offset, n_offset, *ctxt, out);
> >> > maybe_show_relative_offset_change(diff, *ctxt, out);
> >> > emitted = true;
> >> > }
> >> >
> >> >- if (!size_reported && size1 != size2)
> >> >+ if (!size_reported && o_size != n_size)
> >> > {
> >> > if (begin_with_and)
> >> > {
> >> >@@ -606,12 +589,12 @@ represent(const var_diff_sptr &diff,
> >> > out << indent;
> >> > if (is_strict_anonymous_data_member_change)
> >> > out << "anonymous data member ";
> >> >- out << "'" << pretty_representation << "' ";
> >> >+ out << "'" << o_pretty_representation << "' ";
> >> > }
> >> > else
> >> > out << ", ";
> >> >
> >> >- show_numerical_change("size", size1, size2, *ctxt, out);
> >> >+ show_numerical_change("size", o_size, n_size, *ctxt, out);
> >> > maybe_show_relative_size_change(diff, *ctxt, out);
> >> > emitted = true;
> >> > }
> >> >@@ -624,7 +607,7 @@ represent(const var_diff_sptr &diff,
> >> > begin_with_and = false;
> >> > }
> >> > else if (!emitted)
> >> >- out << indent << "'" << pretty_representation << "' ";
> >> >+ out << indent << "'" << o_pretty_representation << "' ";
> >> > else
> >> > out << ", ";
> >> > out << "elf binding changed from " << o->get_binding()
> >> >@@ -639,7 +622,7 @@ represent(const var_diff_sptr &diff,
> >> > begin_with_and = false;
> >> > }
> >> > else if (!emitted)
> >> >- out << indent << "'" << pretty_representation << "' ";
> >> >+ out << indent << "'" << o_pretty_representation << "' ";
> >> > else
> >> > out << ", ";
> >> > out << "visibility changed from " << o->get_visibility()
> >> >@@ -656,7 +639,7 @@ represent(const var_diff_sptr &diff,
> >> > begin_with_and = false;
> >> > }
> >> > else if (!emitted)
> >> >- out << indent << "'" << pretty_representation << "' ";
> >> >+ out << indent << "'" << o_pretty_representation << "' ";
> >> > else
> >> > out << ", ";
> >> >
> >> >@@ -675,7 +658,7 @@ represent(const var_diff_sptr &diff,
> >> > begin_with_and = false;
> >> > }
> >> > else if (!emitted)
> >> >- out << indent << "'" << pretty_representation << "' ";
> >> >+ out << indent << "'" << o_pretty_representation << "' ";
> >> > else
> >> > out << ", ";
> >> >
> >> >@@ -691,7 +674,7 @@ represent(const var_diff_sptr &diff,
> >> > ;
> >> > else if (!emitted)
> >> > // We appear to have fallen off the edge of the map.
> >> >- out << indent << "'" << pretty_representation
> >> >+ out << indent << "'" << o_pretty_representation
> >> > << "' has *some* difference - please report as a bug";
> >> > else
> >> > // do nothing
> >> >--
> >> >2.26.0.110.g2183baf09c-goog
> >> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/2] Improve readability of represent helper function.
2020-04-08 16:38 ` Giuliano Procida
@ 2020-04-08 16:40 ` Giuliano Procida
2020-04-14 11:09 ` Dodji Seketeli
0 siblings, 1 reply; 10+ messages in thread
From: Giuliano Procida @ 2020-04-08 16:40 UTC (permalink / raw)
To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich
This change:
- makes local variables const where possible
- gives local variables more descriptive names
- factors out some more expressions as local variables
- simplifies the logic around anonymous data members
There are no behavioural changes.
* src/abg-reporter-priv.cc (represent): In the var_diff_sptr
overload, rename pretty_representation to
o_pretty_representation, introduce n_pretty_representation
where needed and replace the duplicate tr1 and tr2 with these;
rename all other variables foo1 and foo2 to o_foo and n_foo
respectively, using _type instead of _t; introduce o_anon and
n_anon and use them to make the local variable
is_strict_anonymous_data_member_change const, make ABG_ASSERT
in anonymous data member handling more obvious in the case
where anonymity has changed and allow simplification of
formatting in this case; move declarations of const local
variables above those of the non-const, state-tracking,
variables.
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
src/abg-reporter-priv.cc | 123 +++++++++++++++++----------------------
1 file changed, 53 insertions(+), 70 deletions(-)
diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc
index 5fb5fbc5..3ff22e60 100644
--- a/src/abg-reporter-priv.cc
+++ b/src/abg-reporter-priv.cc
@@ -409,14 +409,21 @@ represent(const var_diff_sptr &diff,
if (!ctxt->get_reporter()->diff_to_be_reported(diff.get()))
return;
- var_decl_sptr o = diff->first_var();
- var_decl_sptr n = diff->second_var();
- string name1 = o->get_qualified_name();
- string name2 = n->get_qualified_name();
- uint64_t size1 = get_var_size_in_bits(o);
- uint64_t size2 = get_var_size_in_bits(n);
- uint64_t offset1 = get_data_member_offset(o);
- uint64_t offset2 = get_data_member_offset(n);
+ const var_decl_sptr o = diff->first_var();
+ const var_decl_sptr n = diff->second_var();
+ const bool o_anon = !!is_anonymous_data_member(o);
+ const bool n_anon = !!is_anonymous_data_member(n);
+ const bool is_strict_anonymous_data_member_change = o_anon && n_anon;
+ const string o_name = o->get_qualified_name();
+ const string n_name = n->get_qualified_name();
+ const uint64_t o_size = get_var_size_in_bits(o);
+ const uint64_t n_size = get_var_size_in_bits(n);
+ const uint64_t o_offset = get_data_member_offset(o);
+ const uint64_t n_offset = get_data_member_offset(n);
+ const string o_pretty_representation = o->get_pretty_representation();
+ // no n_pretty_representation here as it's only needed in a couple of places
+ const bool show_size_offset_changes = ctxt->get_allowed_category()
+ & SIZE_OR_OFFSET_CHANGE_CATEGORY;
// Has the main diff text been output?
bool emitted = false;
@@ -424,44 +431,36 @@ represent(const var_diff_sptr &diff,
bool begin_with_and = false;
// Have we reported a size change already?
bool size_reported = false;
- // Are we reporting a change to an anonymous member?
- bool is_strict_anonymous_data_member_change = false;
- string pretty_representation = o->get_pretty_representation();
- bool show_size_offset_changes = ctxt->get_allowed_category()
- & SIZE_OR_OFFSET_CHANGE_CATEGORY;
-
- if (is_anonymous_data_member(o) && is_anonymous_data_member(n))
+ if (is_strict_anonymous_data_member_change)
{
- is_strict_anonymous_data_member_change = true;
- string tr1 = o->get_pretty_representation();
- string tr2 = n->get_pretty_representation();
- type_base_sptr t1 = o->get_type(), t2 = n->get_type();
- if (tr1 != tr2)
+ const string n_pretty_representation = n->get_pretty_representation();
+ const type_base_sptr o_type = o->get_type(), n_type = n->get_type();
+ if (o_pretty_representation != n_pretty_representation)
{
show_offset_or_size(indent + "anonymous data member at offset",
- offset1, *ctxt, out);
+ o_offset, *ctxt, out);
out << " changed from:\n"
- << indent << " " << tr1 << "\n"
+ << indent << " " << o_pretty_representation << "\n"
<< indent << "to:\n"
- << indent << " " << tr2 << "\n";
+ << indent << " " << n_pretty_representation << "\n";
begin_with_and = true;
emitted = true;
}
- else if (get_type_name(t1) != get_type_name(t2)
- && is_decl(t1) && is_decl(t2)
- && is_decl(t1)->get_is_anonymous()
- && is_decl(t2)->get_is_anonymous())
+ else if (get_type_name(o_type) != get_type_name(n_type)
+ && is_decl(o_type) && is_decl(n_type)
+ && is_decl(o_type)->get_is_anonymous()
+ && is_decl(n_type)->get_is_anonymous())
{
out << indent << "while looking at anonymous data member '"
- << tr1 << "':\n"
+ << o_pretty_representation << "':\n"
<< indent << "the internal name of that anonymous data member"
"changed from:\n"
- << indent << " " << get_type_name(o->get_type()) << "\n"
+ << indent << " " << get_type_name(o_type) << "\n"
<< indent << "to:\n"
- << indent << " " << get_type_name(n->get_type()) << "\n"
+ << indent << " " << get_type_name(n_type) << "\n"
<< indent << " This is usually due to "
<< "an anonymous member type being added or removed from "
<< "the containing type\n";
@@ -472,36 +471,20 @@ represent(const var_diff_sptr &diff,
}
else if (filtering::has_anonymous_data_member_change(diff))
{
+ ABG_ASSERT(o_anon != n_anon);
// So we are looking at a non-anonymous data member change from
- // or to anonymous data member.
- if (is_anonymous_data_member(o))
- {
- string repr = "anonymous data member "
- + o->get_pretty_representation()
- + " at offset";
-
- show_offset_or_size(indent + repr, offset1, *ctxt, out);
- out << " became data member '"
- << n->get_pretty_representation()
- << "'\n";
- }
- else
- {
- ABG_ASSERT(is_anonymous_data_member(n));
- string repr = "data member "
- + o->get_pretty_representation()
- + " at offset";
-
- show_offset_or_size(indent + repr, offset1, *ctxt, out);
- out << " became anonymous data member '"
- << n->get_pretty_representation()
- << "'\n";
- }
+ // or to an anonymous data member.
+ const string n_pretty_representation = n->get_pretty_representation();
+ out << indent << (o_anon ? "anonymous " : "")
+ << "data member " << o_pretty_representation;
+ show_offset_or_size(" at offset", o_offset, *ctxt, out);
+ out << " became " << (n_anon ? "anonymous " : "")
+ << "data member '" << n_pretty_representation << "'\n";
begin_with_and = true;
emitted = true;
}
- else if (diff_sptr d = diff->type_diff())
+ else if (const diff_sptr d = diff->type_diff())
{
if (ctxt->get_reporter()->diff_to_be_reported(d.get()))
{
@@ -511,7 +494,7 @@ represent(const var_diff_sptr &diff,
<< "' changed";
else
out << indent
- << "type of '" << pretty_representation << "' changed";
+ << "type of '" << o_pretty_representation << "' changed";
if (d->currently_reporting())
out << ", as being reported\n";
@@ -529,7 +512,7 @@ represent(const var_diff_sptr &diff,
}
}
- if (!filtering::has_anonymous_data_member_change(diff) && name1 != name2)
+ if (!filtering::has_anonymous_data_member_change(diff) && o_name != n_name)
{
if (filtering::has_harmless_name_change(o, n)
&& !(ctxt->get_allowed_category()
@@ -546,7 +529,7 @@ represent(const var_diff_sptr &diff,
out << indent;
else
out << ", ";
- out << "name of '" << name1 << "' changed to '" << name2 << "'";
+ out << "name of '" << o_name << "' changed to '" << n_name << "'";
report_loc_info(n, *ctxt, out);
emitted = true;
}
@@ -561,7 +544,7 @@ represent(const var_diff_sptr &diff,
begin_with_and = false;
}
else if (!emitted)
- out << indent << "'" << pretty_representation << "' ";
+ out << indent << "'" << o_pretty_representation << "' ";
else
out << ", ";
if (get_data_member_is_laid_out(o))
@@ -572,7 +555,7 @@ represent(const var_diff_sptr &diff,
}
if (show_size_offset_changes)
{
- if (offset1 != offset2)
+ if (o_offset != n_offset)
{
if (begin_with_and)
{
@@ -584,17 +567,17 @@ represent(const var_diff_sptr &diff,
out << indent;
if (is_strict_anonymous_data_member_change)
out << "anonymous data member ";
- out << "'" << pretty_representation << "' ";
+ out << "'" << o_pretty_representation << "' ";
}
else
out << ", ";
- show_numerical_change("offset", offset1, offset2, *ctxt, out);
+ show_numerical_change("offset", o_offset, n_offset, *ctxt, out);
maybe_show_relative_offset_change(diff, *ctxt, out);
emitted = true;
}
- if (!size_reported && size1 != size2)
+ if (!size_reported && o_size != n_size)
{
if (begin_with_and)
{
@@ -606,12 +589,12 @@ represent(const var_diff_sptr &diff,
out << indent;
if (is_strict_anonymous_data_member_change)
out << "anonymous data member ";
- out << "'" << pretty_representation << "' ";
+ out << "'" << o_pretty_representation << "' ";
}
else
out << ", ";
- show_numerical_change("size", size1, size2, *ctxt, out);
+ show_numerical_change("size", o_size, n_size, *ctxt, out);
maybe_show_relative_size_change(diff, *ctxt, out);
emitted = true;
}
@@ -624,7 +607,7 @@ represent(const var_diff_sptr &diff,
begin_with_and = false;
}
else if (!emitted)
- out << indent << "'" << pretty_representation << "' ";
+ out << indent << "'" << o_pretty_representation << "' ";
else
out << ", ";
out << "elf binding changed from " << o->get_binding()
@@ -639,7 +622,7 @@ represent(const var_diff_sptr &diff,
begin_with_and = false;
}
else if (!emitted)
- out << indent << "'" << pretty_representation << "' ";
+ out << indent << "'" << o_pretty_representation << "' ";
else
out << ", ";
out << "visibility changed from " << o->get_visibility()
@@ -656,7 +639,7 @@ represent(const var_diff_sptr &diff,
begin_with_and = false;
}
else if (!emitted)
- out << indent << "'" << pretty_representation << "' ";
+ out << indent << "'" << o_pretty_representation << "' ";
else
out << ", ";
@@ -675,7 +658,7 @@ represent(const var_diff_sptr &diff,
begin_with_and = false;
}
else if (!emitted)
- out << indent << "'" << pretty_representation << "' ";
+ out << indent << "'" << o_pretty_representation << "' ";
else
out << ", ";
@@ -691,7 +674,7 @@ represent(const var_diff_sptr &diff,
;
else if (!emitted)
// We appear to have fallen off the edge of the map.
- out << indent << "'" << pretty_representation
+ out << indent << "'" << o_pretty_representation
<< "' has *some* difference - please report as a bug";
else
// do nothing
--
2.26.0.110.g2183baf09c-goog
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] Improve readability of represent helper function.
2020-04-08 16:40 ` [PATCH v2 " Giuliano Procida
@ 2020-04-14 11:09 ` Dodji Seketeli
0 siblings, 0 replies; 10+ messages in thread
From: Dodji Seketeli @ 2020-04-14 11:09 UTC (permalink / raw)
To: Giuliano Procida; +Cc: libabigail, kernel-team, maennich
Giuliano Procida <gprocida@google.com> a écrit:
> This change:
>
> - makes local variables const where possible
> - gives local variables more descriptive names
> - factors out some more expressions as local variables
> - simplifies the logic around anonymous data members
>
> There are no behavioural changes.
>
> * src/abg-reporter-priv.cc (represent): In the var_diff_sptr
> overload, rename pretty_representation to
> o_pretty_representation, introduce n_pretty_representation
> where needed and replace the duplicate tr1 and tr2 with these;
> rename all other variables foo1 and foo2 to o_foo and n_foo
> respectively, using _type instead of _t; introduce o_anon and
> n_anon and use them to make the local variable
> is_strict_anonymous_data_member_change const, make ABG_ASSERT
> in anonymous data member handling more obvious in the case
> where anonymity has changed and allow simplification of
> formatting in this case; move declarations of const local
> variables above those of the non-const, state-tracking,
> variables.
Applied to master, thanks!
Cheers,
--
Dodji
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] abidiff: Document and refresh tests.
2020-04-08 12:20 ` [PATCH 2/2] abidiff: Document and refresh tests Giuliano Procida
2020-04-08 12:54 ` Matthias Maennich
@ 2020-04-14 11:24 ` Dodji Seketeli
1 sibling, 0 replies; 10+ messages in thread
From: Dodji Seketeli @ 2020-04-14 11:24 UTC (permalink / raw)
To: Giuliano Procida; +Cc: libabigail, kernel-team, maennich
Giuliano Procida <gprocida@google.com> a écrit:
> Following on from giving some test file more descriptive names, this
> patch actually documents in brief the purpose of these recently added
> tests.
>
> It also improves the coverage of the test-leaf-cxx-members test to
> include deletion and insertion report text as well.
>
> * tests/data/test-abidiff-exit/test-leaf-cxx-members-v1.cc:
> Comment test. Reorder members of ops to get better coverage.
> * tests/data/test-abidiff-exit/test-leaf-fun-type-v1.cc:
> Comment test.
> * tests/data/test-abidiff-exit/test-leaf-more-v1.cc: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc:
> Comment test. Update comment on ops2.
> * tests/data/test-abidiff-exit/test-leaf-redundant-v1.c:
> Comment test.
> * tests/data/test-abidiff-exit/test-leaf-stats-v1.cc: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-cxx-members-report.txt:
> Update locations. Update report to reflect deletion,
> insertion and changed members (was previously changed only).:
> * tests/data/test-abidiff-exit/test-leaf-peeling-report.txt:
> Update locations.
> * tests/data/test-abidiff-exit/test-leaf-redundant-report.txt:
> Ditto.:
> * tests/data/test-abidiff-exit/test-leaf-cxx-members-v0.cc:
> Added one line comment referring to -v1 source file.
> * tests/data/test-abidiff-exit/test-leaf-fun-type-v0.cc: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-more-v0.cc: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-redundant-v0.c: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-stats-v0.cc: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-cxx-members-v0.o: Recompiled.
> * tests/data/test-abidiff-exit/test-leaf-cxx-members-v1.o: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-fun-type-v0.o: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-fun-type-v1.o: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-more-v0.o: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-more-v1.o: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-peeling-v0.o: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-peeling-v1.o: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-redundant-v0.o: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-redundant-v1.o: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-stats-v0.o: Ditto.
> * tests/data/test-abidiff-exit/test-leaf-stats-v1.o: Ditto.
Applied to master, thanks!
Cheers,
--
Dodji
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-04-14 11:24 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-08 12:20 [PATCH 1/2] Improve readability of represent helper function Giuliano Procida
2020-04-08 12:20 ` [PATCH 2/2] abidiff: Document and refresh tests Giuliano Procida
2020-04-08 12:54 ` Matthias Maennich
2020-04-14 11:24 ` Dodji Seketeli
2020-04-08 12:47 ` [PATCH 1/2] Improve readability of represent helper function Matthias Maennich
2020-04-08 14:24 ` Giuliano Procida
2020-04-08 14:57 ` Matthias Maennich
2020-04-08 16:38 ` Giuliano Procida
2020-04-08 16:40 ` [PATCH v2 " Giuliano Procida
2020-04-14 11:09 ` Dodji Seketeli
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).