Compact & intuitive reporting for changed function parameters

* include/abg-comparison.h (unsigned_changed_parm_map)
	(unsigned_parm_map): New typedefs.
	* src/abg-comparison.cc
	(function_decl_diff::subtype_changed_parms): Renamed this from
	changed_parms.
	(function_decl_diff::priv::subtype_changed_parms_}): Renamed this
	from changed_parms_;
	(function_decl_diff::priv::{deleted_parms_by_id_,
	added_parms_by_id_, changed_parms_by_id_}): New members.
	(function_decl_diff::ensure_lookup_tables_populated): Update the
	new priv::changed_parms_by_id_ member to contain the parameters
	that got changed, as opposed to the subtype_changed_parms_ member
	that contain parameters that got a sub-type change.  The other
	priv::*_by_id_ members are helpers for this.  Update them too.
	(function_decl_diff::subtype_changed_parms): Renamed this from
	::changed_parms.
	(function_decl_diff::report): Report separately about parameters
	having a sub-type change, and parameters that got changed.  Note
	that in both cases, the change is necessarily to a compatible
	type.
	* data/test-diff-dwarf/test2-v0.cc: New test file.
	* data/test-diff-dwarf/test2-v0.o: Likewise.
	* data/test-diff-dwarf/test2-v1.cc: Likewise.
	* data/test-diff-dwarf/test2-v1.o: Likewise.
	* data/test-diff-dwarf/test2-report.txt: Likewise.
	* tests/Makefile.am: Add the new test files above to the
	distribution.
	* tests/test-diff-dwarf.cc: Consume the input files above to
	actually perform a regression test.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>

include/abg-comparison.h

@@ -89,6 +89,15 @@ typedef pair<function_decl::parameter_sptr,
 /// parameter and which key is the name of the function parameter.
 typedef unordered_map<string, changed_parm> string_changed_parm_map;
 
+/// Convenience typedef for a map which key is an integer and which
+/// value is a changed parameter.
+typedef unordered_map<unsigned, changed_parm> unsigned_changed_parm_map;
+
+/// Convenience typedef for a map which key is an integer and which
+/// value is a parameter.
+typedef unordered_map<unsigned,
+		      function_decl::parameter_sptr> unsigned_parm_map;
+
 /// Convenience typedef for a map which value is changed type of decl.
 /// The key of the map is the qualified name of the type/decl.
 typedef unordered_map<string,
@@ -1101,7 +1110,7 @@ compute_diff(const function_decl_sptr	first,
   return_type_diff() const;
 
   const string_changed_parm_map&
-  changed_parms() const;
+  subtype_changed_parms() const;
 
   const string_parm_map&
   removed_parms() const;

src/abg-comparison.cc

@@ -4246,16 +4246,19 @@ struct function_decl_diff::priv
   };// end enum Flags
 
 
-  diff_sptr return_type_diff_;
-  edit_script parm_changes_;
-  vector<char> first_fn_flags_;
-  vector<char> second_fn_flags_;
-  edit_script fn_flags_changes_;
+  diff_sptr	return_type_diff_;
+  edit_script	parm_changes_;
+  vector<char>	first_fn_flags_;
+  vector<char>	second_fn_flags_;
+  edit_script	fn_flags_changes_;
 
   // useful lookup tables.
-  string_parm_map deleted_parms_;
-  string_changed_parm_map changed_parms_;
-  string_parm_map added_parms_;
+  string_parm_map		deleted_parms_;
+  string_parm_map		added_parms_;
+  string_changed_parm_map	subtype_changed_parms_;
+  unsigned_parm_map		deleted_parms_by_id_;
+  unsigned_parm_map		added_parms_by_id_;
+  unsigned_changed_parm_map	changed_parms_by_id_;
 
   Flags
   fn_is_declared_inline_to_flag(function_decl_sptr f) const
@@ -4332,6 +4335,7 @@ function_decl_diff::ensure_lookup_tables_populated()
       // fix that.
       assert(!parm_name.empty());
       priv_->deleted_parms_[parm_name] = parm;
+      priv_->deleted_parms_by_id_[parm->get_index()] = parm;
     }
 
   for (vector<insertion>::const_iterator i =
@@ -4349,17 +4353,34 @@ function_decl_diff::ensure_lookup_tables_populated()
 	  // If for a reason the type name is empty we want to know and
 	  // fix that.
 	  assert(!parm_name.empty());
-	  string_parm_map::const_iterator k =
-	    priv_->deleted_parms_.find(parm_name);
-	  if (k != priv_->deleted_parms_.end())
-	    {
-	      if (*k->second != *parm)
-		priv_->changed_parms_[parm_name] =
+	  {
+	    string_parm_map::const_iterator k =
+	      priv_->deleted_parms_.find(parm_name);
+	    if (k != priv_->deleted_parms_.end())
+	      {
+		if (*k->second != *parm)
+		  priv_->subtype_changed_parms_[parm_name] =
+		    std::make_pair(k->second, parm);
+		priv_->deleted_parms_.erase(parm_name);
+	      }
+	    else
+	      priv_->added_parms_[parm_name] = parm;
+	  }
+	  {
+	    unsigned_parm_map::const_iterator k =
+	      priv_->deleted_parms_by_id_.find(parm->get_index());
+	    if (k != priv_->deleted_parms_by_id_.end())
+	      {
+		priv_->changed_parms_by_id_[parm->get_index()] =
 		  std::make_pair(k->second, parm);
-	      priv_->deleted_parms_.erase(k);
-	    }
-	  else
-	    priv_->added_parms_[parm_name] = parm;
+		priv_->subtype_changed_parms_.erase(parm_name);
+		priv_->added_parms_.erase(parm_name);
+		priv_->deleted_parms_.erase(k->second->get_name_id());
+		priv_->deleted_parms_by_id_.erase(parm->get_index());
+	      }
+	    else
+	      priv_->added_parms_by_id_[parm->get_index()] = parm;
+	  }
 	}
     }
 }
@@ -4406,8 +4427,8 @@ function_decl_diff::return_type_diff() const
 /// @return a map of the parameters whose type got changed.  The key
 /// of the map is the name of the type.
 const string_changed_parm_map&
-function_decl_diff::changed_parms() const
-{return priv_->changed_parms_;}
+function_decl_diff::subtype_changed_parms() const
+{return priv_->subtype_changed_parms_;}
 
 /// @return a map of parameters that got removed.
 const string_parm_map&
@@ -4463,10 +4484,35 @@ function_decl_diff::report(ostream& out, const string& indent) const
   // Hmmh, the above was quick.  Now report about function
   // parameters; this shouldn't as straightforward.
   //
-  // Report about the parameter types that have changed.
+  // Report about the parameter types that have changed sub-types.
   for (string_changed_parm_map::const_iterator i =
-	 priv_->changed_parms_.begin();
-       i != priv_->changed_parms_.end();
+	 priv_->subtype_changed_parms_.begin();
+       i != priv_->subtype_changed_parms_.end();
+       ++i)
+    {
+      diff_sptr d = compute_diff_for_types(i->second.first->get_type(),
+					   i->second.second->get_type(),
+					   context());
+      if (d)
+	{
+	  if (d->to_be_reported())
+	    {
+	      out << indent
+		  << "parameter " << i->second.first->get_index()
+		  << " of type '"
+		  << i->second.first->get_type_pretty_representation()
+		  << "' has sub-type changes:\n";
+	      d->report(out, indent + "  ");
+	    }
+	}
+    }
+  // Report about parameters that have changed, while staying
+  // compatible -- otherwise they would have changed the mangled name
+  // of the function and the function would have been reported as
+  // removed.
+  for (unsigned_changed_parm_map::const_iterator i =
+	 priv_->changed_parms_by_id_.begin();
+       i != priv_->changed_parms_by_id_.end();
        ++i)
     {
       diff_sptr d = compute_diff_for_types(i->second.first->get_type(),
@@ -4533,8 +4579,22 @@ function_decl_diff::traverse(diff_node_visitor& v)
 	return false;
     }
 
-  for (string_changed_parm_map::const_iterator i = changed_parms().begin();
-       i != changed_parms().end();
+  for (string_changed_parm_map::const_iterator i =
+	 subtype_changed_parms().begin();
+       i != subtype_changed_parms().end();
+       ++i)
+    if (diff_sptr d = compute_diff_for_types(i->second.first->get_type(),
+					     i->second.second->get_type(),
+					     context()))
+      {
+	d->set_parent(this);
+	if (!d->traverse(v))
+	  return false;
+      }
+
+  for (unsigned_changed_parm_map::const_iterator i =
+	 priv_->changed_parms_by_id_.begin();
+       i != priv_->changed_parms_by_id_.end();
        ++i)
     if (diff_sptr d = compute_diff_for_types(i->second.first->get_type(),
 					     i->second.second->get_type(),

tests/Makefile.am

@@ -122,6 +122,11 @@ data/test-diff-dwarf/test1-v0.o			\
 data/test-diff-dwarf/test1-v1.cc		\
 data/test-diff-dwarf/test1-v1.o			\
 data/test-diff-dwarf/test1-report.txt		\
+data/test-diff-dwarf/test2-v0.cc		\
+data/test-diff-dwarf/test2-v0.o			\
+data/test-diff-dwarf/test2-v1.cc		\
+data/test-diff-dwarf/test2-v1.o			\
+data/test-diff-dwarf/test2-report.txt		\
 \
 data/test-read-dwarf/test0			\
 data/test-read-dwarf/test0.abi			\

tests/data/test-diff-dwarf/test2-report.txt

@@ -0,0 +1,11 @@
+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(int, char)' has some indirect sub-type changes:
+    parameter 0 of type 'int' changed:
+      entity changed from int to typedef Int
+    parameter 1 of type 'char' changed:
+      entity changed from char to typedef Char
+

tests/data/test-diff-dwarf/test2-v0.cc

@@ -0,0 +1,4 @@
+void
+foo(int, char)
+{
+}

tests/data/test-diff-dwarf/test2-v1.cc

@@ -0,0 +1,7 @@
+typedef int Int;
+typedef char Char;
+
+void
+foo(Int, Char)
+{
+}

tests/test-diff-dwarf.cc

@@ -68,6 +68,12 @@ InOutSpec in_out_specs[] =
     "data/test-diff-dwarf/test1-report.txt",
     "output/test-diff-dwarf/test1-report.txt"
   },
+  {
+    "data/test-diff-dwarf/test2-v0.o",
+    "data/test-diff-dwarf/test2-v1.o",
+    "data/test-diff-dwarf/test2-report.txt",
+    "output/test-diff-dwarf/test2-report.txt"
+  },
   // This should be the last entry
   {NULL, NULL, NULL, NULL}
 };