From b6d7fcc515d069687e962d46af10ed777b09935a Mon Sep 17 00:00:00 2001 From: Dodji Seketeli Date: Fri, 22 Aug 2014 17:19:27 +0200 Subject: [PATCH] A builtin type name change is not harmless - fix that * include/abg-comp-filter.h (has_harmless_name_change): New function declaration. * include/abg-comparison.h (diff_category::DECL_NAME_CHANGE_CATEGORY): Renamed this into HARMLESS_DECL_NAME_CHANGE_CATEGORY. (diff_category::EVERYTHING_CATEGORY): Update. * include/abg-fwd.h (is_enum): New function declaration. (is_var_decl): Return the shared_ptr rather than a bool. (is_data_member): New overload that takes a shared_ptr. * src/abg-comp-filter.cc (decl_name_changed): Consider the qualified name here. (has_harmless_name_change): Define new function declaration. (harmless_filter::visit): Use the new has_harmless_name_change function. * src/abg-comparison.cc (represent) (report_name_size_and_alignment_changes, enum_diff::report) (typedef_diff::report, is_data_member): Use the new filtering::has_harmless_name_change function to simplify logic of emitting the name change related diff * tools/bidiff.cc (set_diff_context_from_opts): Adjust DECL_NAME_CHANGE_CATEGORY -> HARMLESS_DECL_NAME_CHANGE_CATEGORY. * src/abg-ir.cc (is_data_member, is_enum): New function definitions. (is_var_decl): Return the var_decl_sptr rather than just a bool. * tests/data/test-diff-filter/test13-report.txt: Adjust. * tests/data/test-diff-filter/test6-report.txt: Adjust. Signed-off-by: Dodji Seketeli --- include/abg-comp-filter.h | 3 ++ include/abg-comparison.h | 9 ++-- include/abg-fwd.h | 15 ++++-- src/abg-comp-filter.cc | 29 +++++++++-- src/abg-comparison.cc | 52 +++++++++++-------- src/abg-ir.cc | 38 ++++++++++++-- tests/data/test-diff-filter/test13-report.txt | 1 + tests/data/test-diff-filter/test6-report.txt | 1 + tools/bidiff.cc | 2 +- 9 files changed, 109 insertions(+), 41 deletions(-) diff --git a/include/abg-comp-filter.h b/include/abg-comp-filter.h index 2c4be314..83daed08 100644 --- a/include/abg-comp-filter.h +++ b/include/abg-comp-filter.h @@ -39,6 +39,9 @@ namespace comparison namespace filtering { +bool +has_harmless_name_change(decl_base_sptr f, decl_base_sptr s); + class filter_base; /// Convenience typedef for a shared pointer to filter_base typedef shared_ptr filter_base_sptr; diff --git a/include/abg-comparison.h b/include/abg-comparison.h index b31ac8f2..6ed7bc1c 100644 --- a/include/abg-comparison.h +++ b/include/abg-comparison.h @@ -243,9 +243,10 @@ enum diff_category /// instance a type and its typedefs. COMPATIBLE_TYPE_CHANGE_CATEGORY = 1 << 2, - /// This means that a diff node in the sub-tree carries a - /// declaration name change. - DECL_NAME_CHANGE_CATEGORY = 1 << 3, + /// This means that a diff node in the sub-tree carries a harmless + /// declaration name change. This is set only for name changes for + /// data members and typedefs. + HARMLESS_DECL_NAME_CHANGE_CATEGORY = 1 << 3, /// This means that a diff node in the sub-tree carries an addition /// or removal of a non-virtual member function. @@ -273,7 +274,7 @@ enum diff_category NOT_REDUNDANT_CATEGORY | ACCESS_CHANGE_CATEGORY | COMPATIBLE_TYPE_CHANGE_CATEGORY - | DECL_NAME_CHANGE_CATEGORY + | HARMLESS_DECL_NAME_CHANGE_CATEGORY | NON_VIRT_MEM_FUN_CHANGE_CATEGORY | STATIC_DATA_MEMBER_CHANGE_CATEGORY | SIZE_OR_OFFSET_CHANGE_CATEGORY diff --git a/include/abg-fwd.h b/include/abg-fwd.h index d97c62cb..6206ac56 100644 --- a/include/abg-fwd.h +++ b/include/abg-fwd.h @@ -175,6 +175,9 @@ is_typedef(const shared_ptr); shared_ptr is_typedef(const shared_ptr); +shared_ptr +is_enum(const shared_ptr&); + shared_ptr is_class_type(const shared_ptr); @@ -184,7 +187,8 @@ is_class_type(const shared_ptr); shared_ptr look_through_decl_only_class(shared_ptr); -bool + +shared_ptr is_var_decl(const shared_ptr); bool @@ -246,12 +250,10 @@ bool get_member_is_static(const shared_ptr); void -set_member_is_static(decl_base&, - bool); +set_member_is_static(decl_base&, bool); void -set_member_is_static(shared_ptr, - bool); +set_member_is_static(shared_ptr, bool); bool is_data_member(const var_decl&); @@ -262,6 +264,9 @@ is_data_member(const var_decl*); bool is_data_member(const shared_ptr); +bool +is_data_member(const shared_ptr&); + shared_ptr is_array_def(const shared_ptr decl); diff --git a/src/abg-comp-filter.cc b/src/abg-comp-filter.cc index a6247086..d725e39c 100644 --- a/src/abg-comp-filter.cc +++ b/src/abg-comp-filter.cc @@ -1,6 +1,6 @@ // -*- Mode: C++ -*- // -// Copyright (C) 2013 Red Hat, Inc. +// Copyright (C) 2013-2014 Red Hat, Inc. // // This file is part of the GNU Application Binary Interface Generic // Analysis and Instrumentation Library (libabigail). This library is @@ -254,13 +254,32 @@ decl_name_changed(decl_base_sptr d1, decl_base_sptr d2) string d1_name, d2_name; if (d1) - d1_name = d1->get_name(); + d1_name = d1->get_qualified_name(); if (d2) - d2_name = d2->get_name(); + d2_name = d2->get_qualified_name(); return d1_name != d2_name; } +/// Test if two decls represents a harmless name change. +/// +/// For now, a harmless name change is a name change for a typedef, +/// enum or data member. +/// +/// @param f the first decl to consider in the comparison. +/// +/// @param s the second decl to consider in the comparison. +/// +/// @return true iff decl @p s represents a harmless change over @p f. +bool +has_harmless_name_change(decl_base_sptr f, decl_base_sptr s) +{ + return (decl_name_changed(f, s) + && ((is_typedef(f) && is_typedef(s)) + || (is_data_member(f) && is_data_member(s)) + || (is_enum(f) && is_enum(s)))); +} + /// Test if a class_diff node has non-static members added or /// removed. /// @@ -531,8 +550,8 @@ harmless_filter::visit(diff* d, bool pre) if (is_compatible_change(f, s)) category |= COMPATIBLE_TYPE_CHANGE_CATEGORY; - if (decl_name_changed(f, s)) - category |= DECL_NAME_CHANGE_CATEGORY; + if (has_harmless_name_change(f, s)) + category |= HARMLESS_DECL_NAME_CHANGE_CATEGORY; if (has_non_virtual_mem_fn_change(d)) category |= NON_VIRT_MEM_FUN_CHANGE_CATEGORY; diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc index ac24c231..6e610d3d 100644 --- a/src/abg-comparison.cc +++ b/src/abg-comparison.cc @@ -1342,15 +1342,21 @@ represent(var_decl_sptr o, string name2 = n->get_qualified_name(); string pretty_representation = o->get_pretty_representation(); - if (ctxt->get_allowed_category() & DECL_NAME_CHANGE_CATEGORY - && name1 != name2) + if (name1 != name2) { - if (!emitted) - out << indent << "'" << pretty_representation << "' "; + if (filtering::has_harmless_name_change(o, n) + && !(ctxt->get_allowed_category() + & HARMLESS_DECL_NAME_CHANGE_CATEGORY)) + ; else - out << ", "; - out << "name changed to '" << name2 << "'"; - emitted = true; + { + if (!emitted) + out << indent << "'" << pretty_representation << "' "; + else + out << ", "; + out << "name changed to '" << name2 << "'"; + emitted = true; + } } if (get_data_member_is_laid_out(o) @@ -1535,14 +1541,21 @@ report_name_size_and_alignment_changes(decl_base_sptr first, string fn = first->get_qualified_name(), sn = second->get_qualified_name(); - if (fn != sn - && ctxt->get_allowed_category() & DECL_NAME_CHANGE_CATEGORY) + if (fn != sn) { - if (nl) - out << "\n"; - out << indent << "name changed from '" - << fn << "' to '" << sn << "'"; - nl = true; + if (!(ctxt->get_allowed_category() & HARMLESS_DECL_NAME_CHANGE_CATEGORY) + && filtering::has_harmless_name_change(first, second)) + // This is a harmless name change. but then + // HARMLESS_DECL_NAME_CHANGE_CATEGORY doesn't seem allowed. + ; + else + { + if (nl) + out << "\n"; + out << indent << "name changed from '" + << fn << "' to '" << sn << "'"; + nl = true; + } } nl |= report_size_and_alignment_changes(first, second, ctxt, @@ -2769,13 +2782,6 @@ enum_diff::report(ostream& out, const string& indent) const out << "\n"; maybe_report_diff_for_member(first, second, context(), out, indent); - // name - if (first->get_name() != second->get_name() - && context()->get_allowed_category() & DECL_NAME_CHANGE_CATEGORY) - out << indent << "enum name changed from '" - << first->get_qualified_name() << "' to '" - << second->get_qualified_name() << "'\n"; - //underlying type underlying_type_diff()->report(out, indent); @@ -5884,8 +5890,8 @@ typedef_diff::report(ostream& out, const string& indent) const maybe_report_diff_for_member(f, s, context(), out, indent); - if (f->get_name() != s->get_name() - && context()->get_allowed_category() & DECL_NAME_CHANGE_CATEGORY) + if (filtering::has_harmless_name_change(f, s) + && context()->get_allowed_category() & HARMLESS_DECL_NAME_CHANGE_CATEGORY) { out << indent << "typedef name changed from " << f->get_qualified_name() diff --git a/src/abg-ir.cc b/src/abg-ir.cc index eb925a6a..ecc67195 100644 --- a/src/abg-ir.cc +++ b/src/abg-ir.cc @@ -1595,6 +1595,19 @@ bool is_data_member(const var_decl_sptr d) {return is_at_class_scope(d);} +/// Test if a decl is a data member. +/// +/// @param d the decl to consider. +/// +/// @return true iff @p d is a data member. +bool +is_data_member(const decl_base_sptr& d) +{ + if (var_decl_sptr v = is_var_decl(d)) + return is_data_member(v); + return false; +} + /// Set the offset of a data member into its containing class. /// /// @param m the data member to consider. @@ -2593,6 +2606,24 @@ typedef_decl_sptr is_typedef(const decl_base_sptr d) {return is_typedef(is_type(d));} +/// Test if a decl is an enum_type_decl +/// +/// @param d the decl to test for. +/// +/// @return the enum_type_decl_sptr if @p d is an enum, nil otherwise. +enum_type_decl_sptr +is_enum(const decl_base_sptr& d) +{return dynamic_pointer_cast(d);} + +/// Test if a type is an enum_type_decl +/// +/// @param t the type to test for. +/// +/// @return the enum_type_decl_sptr if @p t is an enum, nil otherwise. +enum_type_decl_sptr +is_enum(const type_base_sptr& t) +{return dynamic_pointer_cast(t);} + /// Test whether a type is a class. /// /// This function looks through typedefs. @@ -2645,10 +2676,11 @@ look_through_decl_only_class(class_decl_sptr klass) /// /// @param decl the decl to test. /// -/// @return true iff decl is a variable declaration. -bool +/// @return the var_decl_sptr iff decl is a variable declaration; nil +/// otherwise. +var_decl_sptr is_var_decl(const shared_ptr decl) -{return decl && dynamic_pointer_cast(decl);} +{return dynamic_pointer_cast(decl);} /// Tests whether a decl is a template parameter composition type. /// diff --git a/tests/data/test-diff-filter/test13-report.txt b/tests/data/test-diff-filter/test13-report.txt index 312d941b..032724e8 100644 --- a/tests/data/test-diff-filter/test13-report.txt +++ b/tests/data/test-diff-filter/test13-report.txt @@ -13,6 +13,7 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable 1 data member change: 'int S::m0' offset changed from 32 to 64 and its type 'int' changed: + name changed from 'int' to 'char' size changed from 32 to 8 bits alignment changed from 32 to 8 bits diff --git a/tests/data/test-diff-filter/test6-report.txt b/tests/data/test-diff-filter/test6-report.txt index 19a450ea..30f26e17 100644 --- a/tests/data/test-diff-filter/test6-report.txt +++ b/tests/data/test-diff-filter/test6-report.txt @@ -6,6 +6,7 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable [C]'function return_type foo()' has some indirect sub-type changes: return type changed: underlying type 'unsigned char' changed: + name changed from 'unsigned char' to 'unsigned int' size changed from 8 to 32 bits alignment changed from 8 to 32 bits diff --git a/tools/bidiff.cc b/tools/bidiff.cc index e6e6f002..5249c890 100644 --- a/tools/bidiff.cc +++ b/tools/bidiff.cc @@ -335,7 +335,7 @@ set_diff_context_from_opts(diff_context_sptr ctxt, ctxt->switch_categories_off (abigail::comparison::ACCESS_CHANGE_CATEGORY | abigail::comparison::COMPATIBLE_TYPE_CHANGE_CATEGORY - | abigail::comparison::DECL_NAME_CHANGE_CATEGORY + | abigail::comparison::HARMLESS_DECL_NAME_CHANGE_CATEGORY | abigail::comparison::NON_VIRT_MEM_FUN_CHANGE_CATEGORY | abigail::comparison::STATIC_DATA_MEMBER_CHANGE_CATEGORY); if (!opts.show_harmful_changes)