From ddfb37ab17ef67cd8dfeb94ad92e3ec5486f70d7 Mon Sep 17 00:00:00 2001 From: Dodji Seketeli Date: Sat, 24 Jan 2015 20:51:16 +0100 Subject: [PATCH] Recognize cyclic diff tree nodes as being redundant Okay I need to introduce some vocabulary here. Suppose we have the version 1 of a library named library-v1.so which source code is: struct S { int m0; struct S* m2; }; int foo(struct S* ptr) { return ptr; } And now suppose we have a version 2 of that library named library-v2.so which source code is modified so that a new data member is inserted into struct S: struct S { int m0; char m1; /* <--- a new data member is inserted here. */ struct S* m2; }; int foo(struct S* ptr) { return ptr; } struct S is said to be a cyclic type because it contains a (data) member which type refers to struct S itself, namely, the type of the data member S::m2 is struct S*, which refers to struct S. So, by analogy, the diff node tree that represents the changes of struct S is also said to be cyclic, for similar reasons: the diff node of the change of S::m2 refers to the diff node of the change of the type of S::m2, namely the diff node of struct S*, which refers to the diff node for the change of struct S itself. Now let's talk about redundancy. When walking the diff node tree of struct S in a depth-first manner, at some point, we look at the diff node for the data member S::m2, and we end up looking at the diff node of its type which is the diff node for struct S*; we keep walking and eventually we look the diff node of the change of the underlying type of struct S, which is the diff node of struct S, and hah! that is a redundant node because it's the first node that we visited when visiting the diff node of ... struct S! So the diff tree node for the change of struct S is not only a cyclic node, it's a redundant diff node as well, and its second occurrence is located at the point of appearance of data member S::m2. Hence the wording "cyclic redundant diff tree node". There! We have our vocabulary all set now. This patch enhances the code of the comparison engine so that a cyclic diff tree node is marked as redundant from the point of its second occurrence, onward. First the patch separates the notion of visiting a diff node from the notion of traversing it. Now traversing a diff node means visiting it and visiting its children nodes. So one can visit a node without traversing it, but one can not traverse a node without visiting it. So, when walking diff node trees, we need to avoid ending up in infinite loop in presence of cyclic nodes. This is why re-traversing a node that is already being traversed is forbidden by this patch, but visiting a node that is being visited is allowed. Before this patch, the notions of visiting and traversing were conflated in one and were not very clear; and one couldn't visit a node that was currently being visited. As a result, in presence of a cyclic node, its redundant nature wasn't being recognized, and so the diff tree node was not being flagged as being redundant. Diff reports were then cluttered by redundant references to changes involving cyclic types. * include/abg-comparison.h (enum visiting_kind): Rename enumerator DO_NOT_MARK_VISITED_NODES_AS_TRAVERSED into DO_NOT_MARK_VISITED_NODES_AS_VISITED. (diff_context::diff_has_been_visited): Rename diff_context::diff_has_been_traversed into this. (diff_context::mark_diff_as_visited): Rename diff_context::mark_diff_as_traversed into this. (diff_context::forget_visited_diffs): Rename diff_context::forget_traversed_diffs into this. (diff_context::forbid_visiting_a_node_twice): Rename diff_context::forbid_traversing_a_node_twice into this. (diff_context::visiting_a_node_twice_is_forbidden): Rename diff_context::traversing_a_node_twice_is_forbidden into this. (diff::is_traversing): Move this from protected to public. * src/abg-comparison.cc (diff_context::priv::visited_diff_nodes_): Rename diff_context::priv::traversed_diff_nodes_ into this. (diff_context::priv::forbid_visiting_a_node_twice_): Rename diff_context::priv::forbid_traversing_a_node_twice_ into this. (diff_context::priv::priv): Adjust. (diff_context::diff_has_been_visited): Rename diff_context::diff_has_been_traversed into this. Adjust. (diff_context::mark_diff_as_visited): Rename diff_context::mark_diff_as_traversed into this. Adjust. (diff_context::forget_visited_diffs): Rename diff_context::forget_traversed_diffs into this. Adjust. (diff_context::forbid_visiting_a_node_twice): Rename diff_context::forbid_traversing_a_node_twice into this. (diff_context::visiting_a_node_twice_is_forbidden): Rename diff_context::traversing_a_node_twice_is_forbidden into this. (diff_context::maybe_apply_filters): Adjust. (diff::end_traversing): Remove the 'mark_as_traversed' parameter of this. Remove the visited-marking code. (diff::traverse): This is the crux of the changes of this patch. Avoid traversing a node that is being traversed, but one can visit a node being visited. Also, traversing a node means visiting it and visiting its children nodes. (diff::is_filtered_out): Simplify logic for filtering redundant code. Basically all nodes that are redundant are filtered. All the complicated logic that was due when diff nodes were shared is not relevant anymore. (corpus_diff::priv::categorize_redundant_changed_sub_nodes) (propagate_categories, apply_suppressions) (diff_node_printer::diff_node_printer, print_diff_tree) (categorize_redundant_changed_sub_nodes) (clear_redundancy_categorization) (clear_redundancy_categorization): Adjust. (redundancy_marking_visitor::visit_begin): Adjust. Also, if the current diff node is already being traversed (that's a clyclic node) then mark it as redundant. * src/abg-comp-filter.cc (apply_filter): Adjust. * tests/data/test-diff-filter/test16-report-2.txt: New test input data. * tests/data/test-diff-filter/libtest25-cyclic-type-v{0,1}.so: New test input binaries. * tests/data/test-diff-filter/test25-cyclic-type-v{0,1}.cc: Source code for the test input binaries. * tests/data/test-diff-filter/test25-cyclic-type-report-0.txt: New test input data. * tests/data/test-diff-filter/test25-cyclic-type-report-1.txt: Likewise. * tests/test-diff-filter.cc (in_out_specs): Add the new test inputs above to the list of test input data over which to run this test harness. * tests/data/Makefile.am: Add the new test files above to source distribution. * tests/data/test-diff-filter/test16-report.txt: Adjust. * tests/data/test-diff-filter/test17-0-report.txt: Likewise. Signed-off-by: Dodji Seketeli --- include/abg-comparison.h | 24 +- src/abg-comp-filter.cc | 12 +- src/abg-comparison.cc | 232 +++++++++--------- tests/data/Makefile.am | 9 +- .../libtest25-cyclic-type-v0.so | Bin 0 -> 8950 bytes .../libtest25-cyclic-type-v1.so | Bin 0 -> 8982 bytes .../data/test-diff-filter/test16-report-2.txt | 17 ++ tests/data/test-diff-filter/test16-report.txt | 2 - .../data/test-diff-filter/test17-0-report.txt | 2 - .../test25-cyclic-type-report-0.txt | 13 + .../test25-cyclic-type-report-1.txt | 15 ++ .../test-diff-filter/test25-cyclic-type-v0.cc | 13 + .../test-diff-filter/test25-cyclic-type-v1.cc | 14 ++ tests/test-diff-filter.cc | 21 ++ 14 files changed, 238 insertions(+), 136 deletions(-) create mode 100755 tests/data/test-diff-filter/libtest25-cyclic-type-v0.so create mode 100755 tests/data/test-diff-filter/libtest25-cyclic-type-v1.so create mode 100644 tests/data/test-diff-filter/test16-report-2.txt create mode 100644 tests/data/test-diff-filter/test25-cyclic-type-report-0.txt create mode 100644 tests/data/test-diff-filter/test25-cyclic-type-report-1.txt create mode 100644 tests/data/test-diff-filter/test25-cyclic-type-v0.cc create mode 100644 tests/data/test-diff-filter/test25-cyclic-type-v1.cc diff --git a/include/abg-comparison.h b/include/abg-comparison.h index 22423a5d..a0fa4f09 100644 --- a/include/abg-comparison.h +++ b/include/abg-comparison.h @@ -254,7 +254,7 @@ enum visiting_kind /// This says that the traversing code should not mark visited nodes /// as having been traversed. This is useful, for instance, for /// visitors which have debugging purposes. - DO_NOT_MARK_VISITED_NODES_AS_TRAVERSED = 1 << 1 + DO_NOT_MARK_VISITED_NODES_AS_VISITED = 1 << 1 }; visiting_kind @@ -792,22 +792,22 @@ public: initialize_canonical_diff(const diff_sptr diff); bool - diff_has_been_traversed(const diff*) const; + diff_has_been_visited(const diff*) const; bool - diff_has_been_traversed(const diff_sptr) const; + diff_has_been_visited(const diff_sptr) const; void - mark_diff_as_traversed(const diff*); + mark_diff_as_visited(const diff*); void - forget_traversed_diffs(); + forget_visited_diffs(); void - forbid_traversing_a_node_twice(bool f); + forbid_visiting_a_node_twice(bool f); bool - traversing_a_node_twice_is_forbidden() const; + visiting_a_node_twice_is_forbidden() const; diff_category get_allowed_category() const; @@ -977,13 +977,8 @@ protected: void begin_traversing(); - bool - is_traversing() const; - void - end_traversing(bool mark_as_traversed = true); - -protected: + end_traversing(); virtual void finish_diff_type(); @@ -1006,6 +1001,9 @@ public: diff* get_canonical_diff() const; + bool + is_traversing() const; + void append_child_node(diff_sptr); diff --git a/src/abg-comp-filter.cc b/src/abg-comp-filter.cc index 3b02b4e8..2129f602 100644 --- a/src/abg-comp-filter.cc +++ b/src/abg-comp-filter.cc @@ -46,10 +46,10 @@ using std::tr1::dynamic_pointer_cast; void apply_filter(filter_base& filter, corpus_diff_sptr d) { - bool s = d->context()->traversing_a_node_twice_is_forbidden(); - d->context()->forbid_traversing_a_node_twice(false); + bool s = d->context()->visiting_a_node_twice_is_forbidden(); + d->context()->forbid_visiting_a_node_twice(false); d->traverse(filter); - d->context()->forbid_traversing_a_node_twice(s); + d->context()->forbid_visiting_a_node_twice(s); } /// Walk a diff sub-tree and apply a filter to the nodes visted. The @@ -62,10 +62,10 @@ apply_filter(filter_base& filter, corpus_diff_sptr d) void apply_filter(filter_base& filter, diff_sptr d) { - bool s = d->context()->traversing_a_node_twice_is_forbidden(); - d->context()->forbid_traversing_a_node_twice(false); + bool s = d->context()->visiting_a_node_twice_is_forbidden(); + d->context()->forbid_visiting_a_node_twice(false); d->traverse(filter); - d->context()->forbid_traversing_a_node_twice(s); + d->context()->forbid_visiting_a_node_twice(s); } /// Walk a diff sub-tree and apply a filter to the nodes visted. The diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc index a540a8cc..5e3c2f4c 100644 --- a/src/abg-comparison.cc +++ b/src/abg-comparison.cc @@ -2273,12 +2273,12 @@ struct diff_context::priv vector canonical_diffs; vector filters_; suppressions_type suppressions_; - pointer_map traversed_diff_nodes_; + pointer_map visited_diff_nodes_; corpus_sptr first_corpus_; corpus_sptr second_corpus_; ostream* default_output_stream_; ostream* error_output_stream_; - bool forbid_traversing_a_node_twice_; + bool forbid_visiting_a_node_twice_; bool show_stats_only_; bool show_soname_change_; bool show_architecture_change_; @@ -2298,7 +2298,7 @@ struct diff_context::priv : allowed_category_(EVERYTHING_CATEGORY), default_output_stream_(), error_output_stream_(), - forbid_traversing_a_node_twice_(true), + forbid_visiting_a_node_twice_(true), show_stats_only_(false), show_soname_change_(true), show_architecture_change_(true), @@ -2598,61 +2598,61 @@ diff_context::initialize_canonical_diff(const diff_sptr diff) /// /// @param d the diff node to consider. bool -diff_context::diff_has_been_traversed(const diff* d) const +diff_context::diff_has_been_visited(const diff* d) const { const diff* canonical = d->get_canonical_diff(); assert(canonical); size_t ptr_value = reinterpret_cast(canonical); - return (priv_->traversed_diff_nodes_.find(ptr_value) - != priv_->traversed_diff_nodes_.end()); + return (priv_->visited_diff_nodes_.find(ptr_value) + != priv_->visited_diff_nodes_.end()); } /// Test if a diff node has been traversed. /// /// @param d the diff node to consider. bool -diff_context::diff_has_been_traversed(const diff_sptr d) const -{return diff_has_been_traversed(d.get());} +diff_context::diff_has_been_visited(const diff_sptr d) const +{return diff_has_been_visited(d.get());} /// Mark a diff node as traversed by a traversing algorithm. /// /// Actually, it's the @ref CanonicalDiff "canonical diff" of this /// node that is marked as traversed. /// -/// Subsequent invocations of diff_has_been_traversed() on the diff +/// Subsequent invocations of diff_has_been_visited() on the diff /// node will yield true. void -diff_context::mark_diff_as_traversed(const diff* d) +diff_context::mark_diff_as_visited(const diff* d) { const diff* canonical = d->get_canonical_diff(); assert(canonical); size_t ptr_value = reinterpret_cast(canonical); - priv_->traversed_diff_nodes_[ptr_value] = true; + priv_->visited_diff_nodes_[ptr_value] = true; } /// Unmark all the diff nodes that were marked as being traversed. void -diff_context::forget_traversed_diffs() -{priv_->traversed_diff_nodes_.clear();} +diff_context::forget_visited_diffs() +{priv_->visited_diff_nodes_.clear();} /// This sets a flag that, if it's true, then during the traversing of -/// a diff nodes tree each node is traversed at most once. +/// a diff nodes tree each node is visited at most once. /// /// @param f if true then during the traversing of a diff nodes tree -/// each node is traversed at most once. +/// each node is visited at most once. void -diff_context::forbid_traversing_a_node_twice(bool f) -{priv_->forbid_traversing_a_node_twice_ = f;} +diff_context::forbid_visiting_a_node_twice(bool f) +{priv_->forbid_visiting_a_node_twice_ = f;} /// Return a flag that, if true, then during the traversing of a diff -/// nodes tree each node is traversed at most once. +/// nodes tree each node is visited at most once. /// /// @return the boolean flag. bool -diff_context::traversing_a_node_twice_is_forbidden() const -{return priv_->forbid_traversing_a_node_twice_;} +diff_context::visiting_a_node_twice_is_forbidden() const +{return priv_->forbid_visiting_a_node_twice_;} /// Getter for the diff tree nodes filters to apply to diff sub-trees. /// @@ -2678,14 +2678,14 @@ diff_context::add_diff_filter(filtering::filter_base_sptr f) /// @param diff the diff sub-tree to apply the filters to. void diff_context::maybe_apply_filters(diff_sptr diff, - bool traverse_nodes_once) + bool visit_nodes_once) { if (get_allowed_category() == EVERYTHING_CATEGORY) return; - bool s = traversing_a_node_twice_is_forbidden(); - if (!traverse_nodes_once) - forbid_traversing_a_node_twice(false); + bool s = visiting_a_node_twice_is_forbidden(); + if (!visit_nodes_once) + forbid_visiting_a_node_twice(false); for (filtering::filters::const_iterator i = diff_filters().begin(); i != diff_filters().end(); @@ -2695,8 +2695,8 @@ diff_context::maybe_apply_filters(diff_sptr diff, propagate_categories(diff); } - if (!traverse_nodes_once) - forbid_traversing_a_node_twice(s); + if (!visit_nodes_once) + forbid_visiting_a_node_twice(s); } /// Apply the diff filters to the diff nodes of a @ref corpus_diff @@ -2709,11 +2709,11 @@ diff_context::maybe_apply_filters(diff_sptr diff, /// @param diff the corpus diff to apply the filters to. void diff_context::maybe_apply_filters(corpus_diff_sptr diff, - bool traverse_nodes_once) + bool visit_nodes_once) { - bool s = traversing_a_node_twice_is_forbidden(); - if (!traverse_nodes_once) - forbid_traversing_a_node_twice(false); + bool s = visiting_a_node_twice_is_forbidden(); + if (!visit_nodes_once) + forbid_visiting_a_node_twice(false); for (filtering::filters::const_iterator i = diff_filters().begin(); i != diff_filters().end(); @@ -2723,8 +2723,8 @@ diff_context::maybe_apply_filters(corpus_diff_sptr diff, propagate_categories(diff); } - if (!traverse_nodes_once) - forbid_traversing_a_node_twice(s); + if (!visit_nodes_once) + forbid_visiting_a_node_twice(s); } /// Getter for the vector of suppressions that specify which diff node @@ -3104,6 +3104,9 @@ diff::diff(decl_base_sptr first_subject, /// given node as being traversed (or not), so that /// diff::is_traversing() can tell if the node is being traversed. /// +/// Note that traversing a node means visiting it *and* visiting its +/// children nodes. +/// /// The canonical node is marked as being traversed too. /// /// These functions are called by the traversing code. @@ -3118,6 +3121,9 @@ diff::begin_traversing() /// Tell if a given node is being traversed or not. /// +/// Note that traversing a node means visiting it *and* visiting its +/// children nodes. +/// /// It's the canonical node which is looked at, actually. /// /// Please read the comments for the diff::begin_traversing() for mode @@ -3134,25 +3140,18 @@ diff::is_traversing() const /// Flag a given diff node as not being traversed anymore. /// +/// Note that traversing a node means visiting it *and* visiting its +/// children nodes. +/// /// Please read the comments of the function diff::begin_traversing() /// for mode context. -/// -/// @param mark_as_traversed if set to true mark the current @ref diff -/// node has having been traversed. That way, subsequent calls to -/// diff_context::diff_has_been_traversed() on the current instance of -/// @ref diff will yield true. void -diff::end_traversing(bool mark_as_traversed) +diff::end_traversing() { assert(is_traversing()); if (priv_->canonical_diff_) priv_->canonical_diff_->priv_->traversing_ = false; priv_->traversing_ = false; - - // Make this node as having been traversed, to allow the detection - // of nodes that might have been visited more than once. - if (mark_as_traversed) - context()->mark_diff_as_traversed(this); } /// Finish the building of a given kind of a diff tree node. @@ -3286,6 +3285,12 @@ diff::reported_once() const /// The generic traversing code that walks a given diff sub-tree. /// +/// Note that there is a difference between traversing a diff node and +/// visiting it. Basically, traversing a diff node means visiting it +/// and visiting its children nodes too. So one can visit a node +/// without traversing it. But traversing a node without visiting it +/// is not possible. +/// /// @param v the entity that visits each node of the diff sub-tree. /// /// @return true to tell the caller that all of the sub-tree could be @@ -3296,51 +3301,58 @@ diff::traverse(diff_node_visitor& v) { finish_diff_type(); - if (context()->traversing_a_node_twice_is_forbidden() - && context()->diff_has_been_traversed(this)) + if (context()->visiting_a_node_twice_is_forbidden() + && context()->diff_has_been_visited(this)) return true; - if (is_traversing()) - return true; - - begin_traversing(); v.visit_begin(this); bool mark_visited_nodes_as_traversed = - !(v.get_visiting_kind() & DO_NOT_MARK_VISITED_NODES_AS_TRAVERSED); + !(v.get_visiting_kind() & DO_NOT_MARK_VISITED_NODES_AS_VISITED); if (!v.visit(this, /*pre=*/true)) { v.visit_end(this); - end_traversing(mark_visited_nodes_as_traversed); + if (mark_visited_nodes_as_traversed) + context()->mark_diff_as_visited(this); return false; } - if (!(v.get_visiting_kind() & SKIP_CHILDREN_VISITING_KIND)) - for (vector::const_iterator i = children_nodes().begin(); - i != children_nodes().end(); - ++i) - { - if (!(*i)->traverse(v)) - { - v.visit_end(this); - end_traversing(mark_visited_nodes_as_traversed); - return false; - } - } + if (!(v.get_visiting_kind() & SKIP_CHILDREN_VISITING_KIND) + && !is_traversing()) + { + begin_traversing(); + for (vector::const_iterator i = children_nodes().begin(); + i != children_nodes().end(); + ++i) + { + if (!(*i)->traverse(v)) + { + v.visit_end(this); + if (mark_visited_nodes_as_traversed) + context()->mark_diff_as_visited(this); + end_traversing(); + return false; + } + } + end_traversing(); + } if (!v.visit(this, /*pref=*/false)) { v.visit_end(this); - end_traversing(mark_visited_nodes_as_traversed); + if (mark_visited_nodes_as_traversed) + context()->mark_diff_as_visited(this); return false; } v.visit_end(this); - end_traversing(mark_visited_nodes_as_traversed); + if (mark_visited_nodes_as_traversed) + context()->mark_diff_as_visited(this); return true; } + /// Sets a flag saying if a report has already been emitted for the /// current diff. /// @@ -3416,17 +3428,11 @@ diff::is_filtered_out() const if (get_category() & SUPPRESSED_CATEGORY) return true; - // We don't want to display redundant top-level function or variable - // diff nodes, when the user asked to avoid seeing redundant diff - // nodes. - if ((dynamic_cast(this) - || (dynamic_cast(this) - // We are talking about top-level variables diff nodes, not - // member variables diff nodes. - && !is_member_decl(first_subject()))) - && !context()->show_redundant_changes()) - if (get_category() & REDUNDANT_CATEGORY) - return true; + // We don't want to display redundant diff nodes, when the user + // asked to avoid seeing redundant diff nodes. + if (!context()->show_redundant_changes() + && (get_category() & REDUNDANT_CATEGORY)) + return true; if (get_category() == NO_CHANGE_CATEGORY) return false; @@ -10467,7 +10473,7 @@ corpus_diff::priv::categorize_redundant_changed_sub_nodes() { diff_sptr diff; - ctxt_->forget_traversed_diffs(); + ctxt_->forget_visited_diffs(); for (function_decl_diff_sptrs_type::const_iterator i = changed_fns_.begin(); i!= changed_fns_.end(); @@ -11802,10 +11808,10 @@ void propagate_categories(diff* diff_tree) { category_propagation_visitor v; - bool s = diff_tree->context()->traversing_a_node_twice_is_forbidden(); - diff_tree->context()->forbid_traversing_a_node_twice(false); + bool s = diff_tree->context()->visiting_a_node_twice_is_forbidden(); + diff_tree->context()->forbid_visiting_a_node_twice(false); diff_tree->traverse(v); - diff_tree->context()->forbid_traversing_a_node_twice(s); + diff_tree->context()->forbid_visiting_a_node_twice(s); } /// Visit all the nodes of a given sub-tree. For each node that has a @@ -11828,10 +11834,10 @@ void propagate_categories(corpus_diff* diff_tree) { category_propagation_visitor v; - bool s = diff_tree->context()->traversing_a_node_twice_is_forbidden(); - diff_tree->context()->forbid_traversing_a_node_twice(false); + bool s = diff_tree->context()->visiting_a_node_twice_is_forbidden(); + diff_tree->context()->forbid_visiting_a_node_twice(false); diff_tree->traverse(v); - diff_tree->context()->forbid_traversing_a_node_twice(s); + diff_tree->context()->forbid_visiting_a_node_twice(s); } /// Visit all the nodes of a given corpus tree. For each node that @@ -11919,10 +11925,10 @@ void apply_suppressions(diff* diff_tree) { suppression_categorization_visitor v; - bool s = diff_tree->context()->traversing_a_node_twice_is_forbidden(); - diff_tree->context()->forbid_traversing_a_node_twice(false); + bool s = diff_tree->context()->visiting_a_node_twice_is_forbidden(); + diff_tree->context()->forbid_visiting_a_node_twice(false); diff_tree->traverse(v); - diff_tree->context()->forbid_traversing_a_node_twice(s); + diff_tree->context()->forbid_visiting_a_node_twice(s); } /// Walk a given diff-sub tree and appply the suppressions carried by @@ -11945,10 +11951,10 @@ void apply_suppressions(const corpus_diff* diff_tree) { suppression_categorization_visitor v; - bool s = diff_tree->context()->traversing_a_node_twice_is_forbidden(); - diff_tree->context()->forbid_traversing_a_node_twice(false); + bool s = diff_tree->context()->visiting_a_node_twice_is_forbidden(); + diff_tree->context()->forbid_visiting_a_node_twice(false); const_cast(diff_tree)->traverse(v); - diff_tree->context()->forbid_traversing_a_node_twice(s); + diff_tree->context()->forbid_visiting_a_node_twice(s); } /// Walk a diff tree and appply the suppressions carried by the @@ -11984,7 +11990,7 @@ struct diff_node_printer : public diff_node_visitor } diff_node_printer(ostream& out) - : diff_node_visitor(DO_NOT_MARK_VISITED_NODES_AS_TRAVERSED), + : diff_node_visitor(DO_NOT_MARK_VISITED_NODES_AS_VISITED), out_(out), level_(0) {} @@ -12073,10 +12079,10 @@ void print_diff_tree(diff* diff_tree, ostream& out) { diff_node_printer p(out); - bool s = diff_tree->context()->traversing_a_node_twice_is_forbidden(); - diff_tree->context()->forbid_traversing_a_node_twice(false); + bool s = diff_tree->context()->visiting_a_node_twice_is_forbidden(); + diff_tree->context()->forbid_visiting_a_node_twice(false); diff_tree->traverse(p); - diff_tree->context()->forbid_traversing_a_node_twice(s); + diff_tree->context()->forbid_visiting_a_node_twice(s); } /// Emit a textual representation of a @ref corpus_diff tree to an @@ -12091,10 +12097,10 @@ void print_diff_tree(corpus_diff* diff_tree, std::ostream& out) { diff_node_printer p(out); - bool s = diff_tree->context()->traversing_a_node_twice_is_forbidden(); - diff_tree->context()->forbid_traversing_a_node_twice(false); + bool s = diff_tree->context()->visiting_a_node_twice_is_forbidden(); + diff_tree->context()->forbid_visiting_a_node_twice(false); diff_tree->traverse(p); - diff_tree->context()->forbid_traversing_a_node_twice(s); + diff_tree->context()->forbid_visiting_a_node_twice(s); } /// Emit a textual representation of a @ref diff sub-tree to an @@ -12146,7 +12152,9 @@ struct redundancy_marking_visitor : public diff_node_visitor { // A diff node that carries a change and that has been already // traversed elsewhere is considered redundant. - if (d->context()->diff_has_been_traversed(d) && d->length()) + if ((d->context()->diff_has_been_visited(d) + || d->get_canonical_diff()->is_traversing()) + && d->length()) { // But if two diff nodes are redundant sibbling, do not // mark them as being redundant. This is to avoid marking @@ -12299,10 +12307,10 @@ categorize_redundancy(diff* diff_tree) if (diff_tree->context()->show_redundant_changes()) return; redundancy_marking_visitor v; - bool s = diff_tree->context()->traversing_a_node_twice_is_forbidden(); - diff_tree->context()->forbid_traversing_a_node_twice(false); + bool s = diff_tree->context()->visiting_a_node_twice_is_forbidden(); + diff_tree->context()->forbid_visiting_a_node_twice(false); diff_tree->traverse(v); - diff_tree->context()->forbid_traversing_a_node_twice(s); + diff_tree->context()->forbid_visiting_a_node_twice(s); } /// Walk a given @ref diff sub-tree to categorize each of the nodes @@ -12321,11 +12329,11 @@ void categorize_redundancy(corpus_diff* diff_tree) { redundancy_marking_visitor v; - diff_tree->context()->forget_traversed_diffs(); - bool s = diff_tree->context()->traversing_a_node_twice_is_forbidden(); - diff_tree->context()->forbid_traversing_a_node_twice(false); + diff_tree->context()->forget_visited_diffs(); + bool s = diff_tree->context()->visiting_a_node_twice_is_forbidden(); + diff_tree->context()->forbid_visiting_a_node_twice(false); diff_tree->traverse(v); - diff_tree->context()->forbid_traversing_a_node_twice(s); + diff_tree->context()->forbid_visiting_a_node_twice(s); } /// Walk a given @ref corpus_diff tree to categorize each of the nodes @@ -12346,11 +12354,11 @@ void clear_redundancy_categorization(diff* diff_tree) { redundancy_clearing_visitor v; - bool s = diff_tree->context()->traversing_a_node_twice_is_forbidden(); - diff_tree->context()->forbid_traversing_a_node_twice(false); + bool s = diff_tree->context()->visiting_a_node_twice_is_forbidden(); + diff_tree->context()->forbid_visiting_a_node_twice(false); diff_tree->traverse(v); - diff_tree->context()->forbid_traversing_a_node_twice(s); - diff_tree->context()->forget_traversed_diffs(); + diff_tree->context()->forbid_visiting_a_node_twice(s); + diff_tree->context()->forget_visited_diffs(); } /// Walk a given @ref diff sub-tree to clear the REDUNDANT_CATEGORY @@ -12369,11 +12377,11 @@ void clear_redundancy_categorization(corpus_diff* diff_tree) { redundancy_clearing_visitor v; - bool s = diff_tree->context()->traversing_a_node_twice_is_forbidden(); - diff_tree->context()->forbid_traversing_a_node_twice(false); + bool s = diff_tree->context()->visiting_a_node_twice_is_forbidden(); + diff_tree->context()->forbid_visiting_a_node_twice(false); diff_tree->traverse(v); - diff_tree->context()->forbid_traversing_a_node_twice(s); - diff_tree->context()->forget_traversed_diffs(); + diff_tree->context()->forbid_visiting_a_node_twice(s); + diff_tree->context()->forget_visited_diffs(); } /// Walk a given @ref corpus_diff tree to clear the REDUNDANT_CATEGORY diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am index 7637b911..281b0b9f 100644 --- a/tests/data/Makefile.am +++ b/tests/data/Makefile.am @@ -307,7 +307,8 @@ test-diff-filter/test16-v0.cc \ test-diff-filter/test16-v1.cc \ test-diff-filter/test16-v0.o \ test-diff-filter/test16-v1.o \ -test-diff-filter/test16-report.txt \ +test-diff-filter/test16-report.txt \ +test-diff-filter/test16-report-2.txt \ test-diff-filter/test17-v0.cc \ test-diff-filter/test17-v1.cc \ test-diff-filter/test17-v0.o \ @@ -354,6 +355,12 @@ test-diff-filter/test24-compatible-vars-report-0.txt \ test-diff-filter/test24-compatible-vars-report-1.txt \ test-diff-filter/test24-compatible-vars-v0.c \ test-diff-filter/test24-compatible-vars-v1.c \ +test-diff-filter/libtest25-cyclic-type-v0.so \ +test-diff-filter/libtest25-cyclic-type-v1.so \ +test-diff-filter/test25-cyclic-type-report-0.txt \ +test-diff-filter/test25-cyclic-type-report-1.txt \ +test-diff-filter/test25-cyclic-type-v0.cc \ +test-diff-filter/test25-cyclic-type-v1.cc \ \ test-diff-suppr/test0-type-suppr-v0.cc \ test-diff-suppr/test0-type-suppr-v1.cc \ diff --git a/tests/data/test-diff-filter/libtest25-cyclic-type-v0.so b/tests/data/test-diff-filter/libtest25-cyclic-type-v0.so new file mode 100755 index 0000000000000000000000000000000000000000..39737d81a3c00a0fc38e36766d4c533bf2dda873 GIT binary patch literal 8950 zcmeHMZ)_Y#6`#HH+2@{pzStxrPOB_!67q*z|H&V@P2JdW?3{y>wvMfqO4eq5yT04# z{+PSH)UGHEBn1L;<@O6fLVVyuKzu+5Rg^|ZbqR{{0SOlXYXKTxDq?iH9okRs?TF@JOy=#gIsb zU6s{=#Q$wkF3a7*jwIS*dg5Kj#QXv}ts?JtVMda8PXbR+gvCdTm?$0<{%ZA0kLVAQ z;&%AGeFEwT!vECSaTbqU`Qht&Dx1&gMaL*Oy3TYvXFE)v0>Sj*u`{}96|9t9bgaVInSo3`XN?(? z8OtrJUZN*w46MM&*w2CC>5KZPs9_*u6pL1o>CYUR%I8PB&asR=S#-?g!Gl^cuN_4- zTh67DNxg_#7vf15*Dcd<45sUo#iHA2rdv6aof;WFF`##A-P#ej)4M4EBkJxC$Ctvc zLoto(QoG&ASb*IY6nfw3{hSv4wJ6?a!aD`NEa9XtwWz#CG%JS+$S-?v9P^Y`JUG3p zShniHy+e52gNvr{s%Ax!z|$j3@T5C$7GmxV#>H zHZXA$9M)kWIQ;9x#xm9i_Fb<10gwHg>%qj;pC{h@^YMhTn0RZw#K~KK)rjO@137W^ z*2bTZf2ns3Kvw#6;&Sh6m>UOwP80Zlr9ovY6VRlUG+ZWbg>FN!^0%8)n6#E%SD?uZ zZn^h77&2!sG*0LBFF5Vvz&{Nz)U~lVapx(hpk>!3w7>INp66~7SD#;>xH&b?`%-kB zvNL!LV+KBh#Sb?&Fixxhm_Omt&?!UfawKS!0OCRFG>0}Nxv?1+!tV?^}X1BmDjmD+~V>+Zl};`j%*h? zt?zJN#YE+Q|8OIZI_=wN4VD&*w|Xpu)F~EQw4VR13&Tp^P1}VmuldA!jMiv;t_K@h z7l`dC`=sFY2*-rah(#i`b3xd_VwNZJ2g?ZF{-=dKEEl*={k$slM@9KRgS3)%JEV4Zb{+0~xT{OuH)@$`!f;$X{_uYG8K}(d zZ@1UvJ(%ok4gj}X!ir6K98lGWfpaXT)SZRWV78Mf^_YWfH&YtMAs1$Kbs;-09 zi5!b4@F^tN%z|N5hTD2z=+*6Jk;r3_Alz@L1+v3nQm-k;ceqA5UQOTjZ=TqJ<)3C?5Gl;Dica^DcyczGv6FFUcG`+x?9`G; zFox6^p!*9(l*<9wjwv+>9qD}5>M-->1-m0PDep%(|A8zmNno>?o{X zE97}wjI5mm|9PktE*f_Q`)Q^n^VzHggH$uE$x;gHFmfr_uM5@AO(EYcnw%_H7fH+5 zIg9epG47-PkFFS}y?fYN-^Iv@2w$W!4RZg0SVr-c;y1}hg(bx~ilbCP4`eWQ6QANc z$sQ3v(J$H%pW<#GU|2WtX}%yy^8=XhMDdvP1Y|H)6QANUNt(B){bWZHV-CiC(kcFv zRD~mKKl-Nj3z(UZp?gD&<{Od=2tp$N#HahuL7A6N^A|~)?~spkrp%uK4BJKXE6wjD z>HfilC-mh_MNqu_c`;v*B>%*Ti44gvN__h6O!5OFN<4zENPPP4Mv~@fvX|SBQ6Jk+ z&rkl&OY4+c{A&__S#%^xlZZl6{}cRO$VA~!&zHW7(f423Uen&SfhOC_ z`VHXX{geMC@ttkyWAfjS__W?!{+Rq<0w24K{L^}bzVA~Uq31^Jr}6W)f@9gY*^1p#M$+gd{$$rMwAk zF_B#J3^PeAYD9>+x zJj`gm^5d1~Nmw3)nT+NazkQ^9Zt2IXoX7Bbn#<>Ue)}k+`0dAA%KKYC9%B@D{rDC} z@zjsEGK!OaybaDZRWHQzLD;QQT=NzN;k>FGuRH}oINzc;#8<_8MtHx#pKAD9yq+lQqYnb18M#z=r^DWtH#UNDX^D(Nd+l zQT;OD4_3jy12~@J%H#Y8fLA;BeO1`Y=f1CVyz<<4f!9+d{v-=d(J8@+RuT?JPY;ah zBg5y$;6V58`K|6`^(3C)78xDfdLUgl^Li?ipENSM>EsJV-6+j4*bq)6+W}$$2x1TH#QA<6lL2cX&&tOk%!!-ZtXaB*1<`GfDuLjS?`r3q^WF2^*W9_!7ly}1LW%+$RCpA~nrn2C0U5E-DHRk0&SkmLPkpFkx9Ho$y<~>mbWNj3@pR02@NRe ze@(^*Yj^4;(%Q_q}xG$~Uin?&@>j8U9A3_wdrS7f*f|Lvcn81<1sg%-#JIs@esufa0v;M9(_{QK(Q zc}9gCA$2|qzC2KT_$0Dei}YpQU1UmZ2-yGG!x z@J8_E>v}p@$m=D?C_1_hx}CQj(5F#=K00woH?5+Two8sxoH#U;E#$2UV=8O8akXRg z)Qmw17+L#SbUb)UAD1Z%WsOqFDuMp=&gnv7yyqxn?WvMurgrSmN(F5XnYoIVPNnn` zSv`cOTwJ$I!!bbDr%ENa(4bp+6Ap}x?i$Y?9}mUz)r;eL=LPY&vCwx|hZb zdI$3#E-%wKv51u8C@IjGNI`ja9F&=uvZ1v(d;wc;6Jo@>fShh}h{i^uYY()^R{!e3?VhkCK#=aaHYHJ$Cgow8iR$5@C1y)*Mr3F@6;Q!bHynf>K z5*LNnNwi>~@PaFFb>@bb0`{u``=YdIUqFG^_j3DHS?8{EqbvKk4btX0a=o;9eMj>u z1&;swhsy=BdEdrsu#8;1HBlsnZn@av_5AN#7+3mkSSw{^%_rAmyhamqJ^JvvKyFXr zlakjbGiH27E)uz%bJ7nNvm$VQxQr0xe^UD6azWVK&bOrfh>ZWIv@7M5>HV(-@0)}9 zUX<>Ck@BN6Ax{JPh(}s9vN_rrQKK88tCf!C z72g6!5*f7W0joV-HFs} zDr=__&g@AmajHj4rO+8eONgtgwkl^rxK~rUGKHMgWfsg6c30YVx^U-bOxbC}&UO_o zvz#}Lypu5P>FF*?y3|FxL7@sTF>PmYr|Q{YV$X;V?bvbG&Jj=KoO0gkPg{AbXs6IL zim6QhOy8cwo?Y=ox_(Vk8KVfGm1c8}F@@48x-!FBzTj9|I$zeN%68UF*d_=oW0W$W znX`Fhy3#4SF{i9z$u8u5hK_MXD{BxzYA3S}XkxprIo1s7V$-f!8GX8F-ba^Dkhk_|QaFxZ?&bRmkNm?7Nz2O_kG_hLKO> zj$WE}ewz4h)YMebI>lDj&ReWw!?^GNKX}qO?%m_oJ2&Gj2N0vQna}eHOP)W_MFbwN*-oNDV>$DA z{AS7X8ke8_SRO+@%?)hx{J=6UGotcS-duhOgNX{)jWC{%Sk4iMf%9iR*FTSOUOvxn zEO|a8KFz5?{tRMNE}nmR{%6VcM;8$&FK;NJ;pJbD^9f7NpE)t9u>3-R&*#!C|Hi}s zkKwZcKA+36goCY;{HD$$XZ-#~5$@fzO9!26yAXi+oCp%g^V_TP2_K z;R0|y*j_}1^6zFqO6JpA%Nxj-0?XSTT|~&ogv>vfF7qnB2vlJhc~?P?&yfayBbd8$}Cp3l85D$bVtUHNgfvj6hq z4Z!oMA8!PnC;fO6@cie;n=A919}ff1SAM+uzKP;hoXL28@%u+A_nLmZ#(j-Qr>%1D z=l72SkKca0y|VxH<1yfI*N?9T9#8#v2kso>1Z^R1<)j z$m*t3D3)}iJOj8LJejo|%hdMl+|`RI*KiV-$?k3wuWFs*EV%nqvz*J#BE@5g+h?C^ z-2((&9~mD!G^`IF-cQ%oqDoRfF!uPq!7=^uk&&ar6Z*v9zOiBQ!29cpw^=Ia86$7f sP4@n$4i6q09YPQPO?a(~>;Kxl_`rbo#@ctCUAY_=7u$T{EjIf92Bj=B=>Px# literal 0 HcmV?d00001 diff --git a/tests/data/test-diff-filter/test16-report-2.txt b/tests/data/test-diff-filter/test16-report-2.txt new file mode 100644 index 00000000..86b16942 --- /dev/null +++ b/tests/data/test-diff-filter/test16-report-2.txt @@ -0,0 +1,17 @@ +Functions changes summary: 0 Removed, 1 Changed, 0 Added function +Variables changes summary: 0 Removed, 0 Changed, 0 Added variable + +1 function with some indirect sub-type change: + + [C]'function void foo(S&)' has some indirect sub-type changes: + parameter 0 of type 'S&' has sub-type changes: + in referenced type 'struct S': + size changed from 64 to 128 bits + 1 data member insertion: + 'int S::m0', at offset 0 (in bits) + 1 data member change: + 'S* S::m2' offset changed from 0 to 64 + and its type 'S*' changed: + pointed to type 'struct S' changed; details are being reported + + diff --git a/tests/data/test-diff-filter/test16-report.txt b/tests/data/test-diff-filter/test16-report.txt index 86b16942..e5717497 100644 --- a/tests/data/test-diff-filter/test16-report.txt +++ b/tests/data/test-diff-filter/test16-report.txt @@ -11,7 +11,5 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable 'int S::m0', at offset 0 (in bits) 1 data member change: 'S* S::m2' offset changed from 0 to 64 - and its type 'S*' changed: - pointed to type 'struct S' changed; details are being reported diff --git a/tests/data/test-diff-filter/test17-0-report.txt b/tests/data/test-diff-filter/test17-0-report.txt index 86ce6648..524410a9 100644 --- a/tests/data/test-diff-filter/test17-0-report.txt +++ b/tests/data/test-diff-filter/test17-0-report.txt @@ -11,7 +11,5 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable 'int S::m0', at offset 0 (in bits) 1 data member change: 'S* S::m2' offset changed from 0 to 64 - and its type 'S*' changed: - pointed to type 'struct S' changed; details are being reported diff --git a/tests/data/test-diff-filter/test25-cyclic-type-report-0.txt b/tests/data/test-diff-filter/test25-cyclic-type-report-0.txt new file mode 100644 index 00000000..df742e74 --- /dev/null +++ b/tests/data/test-diff-filter/test25-cyclic-type-report-0.txt @@ -0,0 +1,13 @@ +Functions changes summary: 0 Removed, 1 Changed, 0 Added function +Variables changes summary: 0 Removed, 0 Changed, 0 Added variable + +1 function with some indirect sub-type change: + + [C]'function void foo(S&)' has some indirect sub-type changes: + parameter 0 of type 'S&' has sub-type changes: + in referenced type 'struct S': + 1 data member insertion: + 'char S::m1', at offset 32 (in bits) + no data member change (1 filtered); + + diff --git a/tests/data/test-diff-filter/test25-cyclic-type-report-1.txt b/tests/data/test-diff-filter/test25-cyclic-type-report-1.txt new file mode 100644 index 00000000..c5076424 --- /dev/null +++ b/tests/data/test-diff-filter/test25-cyclic-type-report-1.txt @@ -0,0 +1,15 @@ +Functions changes summary: 0 Removed, 1 Changed, 0 Added function +Variables changes summary: 0 Removed, 0 Changed, 0 Added variable + +1 function with some indirect sub-type change: + + [C]'function void foo(S&)' has some indirect sub-type changes: + parameter 0 of type 'S&' has sub-type changes: + in referenced type 'struct S': + 1 data member insertion: + 'char S::m1', at offset 32 (in bits) + 1 data member change: + type of 'S* S::m2' changed: + pointed to type 'struct S' changed; details are being reported + + diff --git a/tests/data/test-diff-filter/test25-cyclic-type-v0.cc b/tests/data/test-diff-filter/test25-cyclic-type-v0.cc new file mode 100644 index 00000000..971201e9 --- /dev/null +++ b/tests/data/test-diff-filter/test25-cyclic-type-v0.cc @@ -0,0 +1,13 @@ +// Compile with: +// g++ -g -shared -o libtest25-cyclic-type-v0.so test25-cyclic-type-v0.cc + +struct S +{ + int m0; + S* m2; +}; + +void +foo(S&) +{ +} diff --git a/tests/data/test-diff-filter/test25-cyclic-type-v1.cc b/tests/data/test-diff-filter/test25-cyclic-type-v1.cc new file mode 100644 index 00000000..1ecc39e4 --- /dev/null +++ b/tests/data/test-diff-filter/test25-cyclic-type-v1.cc @@ -0,0 +1,14 @@ +// Compile with: +// g++ -g -shared -o libtest25-cyclic-type-v1.so test25-cyclic-type-v1.cc + +struct S +{ + int m0; + char m1; + S* m2; +}; + +void +foo(S&) +{ +} diff --git a/tests/test-diff-filter.cc b/tests/test-diff-filter.cc index d924cebb..a05085f2 100644 --- a/tests/test-diff-filter.cc +++ b/tests/test-diff-filter.cc @@ -194,6 +194,13 @@ InOutSpec in_out_specs[] = "data/test-diff-filter/test16-report.txt", "output/test-diff-filter/test16-report.txt", }, + { + "data/test-diff-filter/test16-v0.o", + "data/test-diff-filter/test16-v1.o", + "--redundant", + "data/test-diff-filter/test16-report-2.txt", + "output/test-diff-filter/test16-report-2.txt", + }, { "data/test-diff-filter/test17-v0.o", "data/test-diff-filter/test17-v1.o", @@ -292,6 +299,20 @@ InOutSpec in_out_specs[] = "data/test-diff-filter/test24-compatible-vars-report-1.txt ", "output/test-diff-filter/test24-compatible-vars-report-1.txt ", }, + { + "data/test-diff-filter/libtest25-cyclic-type-v0.so", + "data/test-diff-filter/libtest25-cyclic-type-v1.so", + "", + "data/test-diff-filter/test25-cyclic-type-report-0.txt ", + "output/test-diff-filter/test25-cyclic-type-report-0.txt " + }, + { + "data/test-diff-filter/libtest25-cyclic-type-v0.so", + "data/test-diff-filter/libtest25-cyclic-type-v1.so", + "--redundant", + "data/test-diff-filter/test25-cyclic-type-report-1.txt ", + "output/test-diff-filter/test25-cyclic-type-report-1.txt " + }, // This should be the last entry {NULL, NULL, NULL, NULL, NULL} };