diff mbox series

abidiff: improve whitespace generation in symbol diff report

Message ID 20211126110642.4164044-1-maennich@google.com
State New
Headers
Series abidiff: improve whitespace generation in symbol diff report |

Commit Message

Matthias Männich Nov. 26, 2021, 11:06 a.m. UTC 
  maybe_report_diff_for_symbol() has a few issues:

1. The responsibility for newline emission is somewhat unclear and
   indeed it would emit spurious blank lines before most of the
   sub-diffs it reports.

2. Different sub-diff text and terminal commas are emitted according to
   whether or not there had been a previous sub-diff - making the output
   harder to grep and post-process.

3. The function also returns a bool but that return value is never used.

Hence, change the function to return void, the function stanzas to
always emit newline-terminated lines and ensure the wording and
punctuation of each sub-diff do not vary. This also tweaks (shortens)
the wording used for CRC diffs.

	* src/abg-reporter-priv.cc (maybe_report_diff_for_symbol):
	Make return void. Simplify and fix new-line emission. Remove
	comma emission. Tweak CRC wording.
	* src/abg-reporter-priv.h (maybe_report_diff_for_symbol):
	Return void.
	* tests/data/test-abidiff-exit/test-crc-report.txt: Shorten CRC
	wording.
	* tests/data/test-abidiff/test-crc-report.txt: Likewise.
	* tests/data/test-diff-filter/test-PR27569-report-0.txt:
	Likewise.

Reviewed-by: Giuliano Procida <gprocida@google.com>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
 src/abg-reporter-priv.cc                      | 88 +++++--------------
 src/abg-reporter-priv.h                       |  2 +-
 .../test-abidiff-exit/test-crc-report.txt     |  6 +-
 tests/data/test-abidiff/test-crc-report.txt   |  3 +-
 .../test-PR27569-report-0.txt                 |  3 +-
 5 files changed, 25 insertions(+), 77 deletions(-)

Comments

Dodji Seketeli Jan. 19, 2022, 2:04 p.m. UTC | #1 
Matthias Maennich <maennich@google.com> a écrit:

> maybe_report_diff_for_symbol() has a few issues:
>
> 1. The responsibility for newline emission is somewhat unclear and
>    indeed it would emit spurious blank lines before most of the
>    sub-diffs it reports.
>
> 2. Different sub-diff text and terminal commas are emitted according to
>    whether or not there had been a previous sub-diff - making the output
>    harder to grep and post-process.
>
> 3. The function also returns a bool but that return value is never used.
>
> Hence, change the function to return void, the function stanzas to
> always emit newline-terminated lines and ensure the wording and
> punctuation of each sub-diff do not vary. This also tweaks (shortens)
> the wording used for CRC diffs.
>
> 	* src/abg-reporter-priv.cc (maybe_report_diff_for_symbol):
> 	Make return void. Simplify and fix new-line emission. Remove
> 	comma emission. Tweak CRC wording.
> 	* src/abg-reporter-priv.h (maybe_report_diff_for_symbol):
> 	Return void.
> 	* tests/data/test-abidiff-exit/test-crc-report.txt: Shorten CRC
> 	wording.
> 	* tests/data/test-abidiff/test-crc-report.txt: Likewise.
> 	* tests/data/test-diff-filter/test-PR27569-report-0.txt:
> 	Likewise.
>
> Reviewed-by: Giuliano Procida <gprocida@google.com>
> Signed-off-by: Matthias Maennich <maennich@google.com>

Applied to master, thanks!

[...]

Cheers,
diff mbox series

Patch

diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc
index 3091f6ca4b83..7012f5dcdce0 100644
--- a/src/abg-reporter-priv.cc
+++ b/src/abg-reporter-priv.cc
@@ -1081,20 +1081,15 @@  maybe_report_diff_for_member(const decl_base_sptr&	decl1,
 /// @param the output stream to emit the report to.
 ///
 /// @param indent the indentation string to use.
-///
-/// @return true if a report was emitted to the output stream @p out,
-/// false otherwise.
-bool
+void
 maybe_report_diff_for_symbol(const elf_symbol_sptr&	symbol1,
 			     const elf_symbol_sptr&	symbol2,
 			     const diff_context_sptr&	ctxt,
 			     ostream&			out,
 			     const string&		indent)
 {
-  bool reported = false;
-
   if (!symbol1 || !symbol2 || symbol1 == symbol2)
-    return reported;
+    return;
 
   if (symbol1->get_size() != symbol2->get_size())
     {
@@ -1104,108 +1099,65 @@  maybe_report_diff_for_symbol(const elf_symbol_sptr&	symbol1,
 			    symbol2->get_size(),
 			    *ctxt, out,
 			    /*show_bits_or_bytes=*/false);
-      reported = true;
+      out << "\n";
     }
 
   if (symbol1->get_name() != symbol2->get_name())
     {
-      if (reported)
-	out << ",\n" << indent
-	    << "its name ";
-      else
-	out << "\n" << indent << "name of symbol ";
-
-      out << "changed from "
+      out << indent << "symbol name changed from "
 	  << symbol1->get_name()
 	  << " to "
-	  << symbol2->get_name();
-
-      reported = true;
+	  << symbol2->get_name()
+	  << "\n";
     }
 
   if (symbol1->get_type() != symbol2->get_type())
     {
-      if (reported)
-	out << ",\n" << indent
-	    << "its type ";
-      else
-	out << "\n" << indent << "type of symbol ";
-
-      out << "changed from '"
+      out << indent << "symbol type changed from '"
 	  << symbol1->get_type()
 	  << "' to '"
 	  << symbol2->get_type()
-	  << "'";
-
-      reported = true;
+	  << "'\n";
     }
 
   if (symbol1->is_public() != symbol2->is_public())
     {
-      if (reported)
-	out << ",\n" << indent
-	    << "it became ";
-	else
-	  out << "\n" << indent << "symbol became ";
-
+      out << indent << "symbol became ";
       if (symbol2->is_public())
 	out << "exported";
       else
 	out << "non-exported";
-
-      reported = true;
+      out << "\n";
     }
 
   if (symbol1->is_defined() != symbol2->is_defined())
     {
-      if (reported)
-	out << ",\n" << indent
-	    << "it became ";
-      else
-	out << "\n" << indent << "symbol became ";
-
+      out << indent << "symbol became ";
       if (symbol2->is_defined())
 	out << "defined";
       else
 	out << "undefined";
-
-      reported = true;
+      out << "\n";
     }
 
   if (symbol1->get_version() != symbol2->get_version())
     {
-      if (reported)
-	out << ",\n" << indent
-	    << "its version changed from ";
-      else
-	out << "\n" << indent << "symbol version changed from ";
-
-      out << symbol1->get_version().str()
+      out << indent << "symbol version changed from "
+	  << symbol1->get_version().str()
 	  << " to "
-	  << symbol2->get_version().str();
-
-      reported = true;
+	  << symbol2->get_version().str()
+	  << "\n";
     }
 
   if (symbol1->get_crc() != 0 && symbol2->get_crc() != 0
       && symbol1->get_crc() != symbol2->get_crc())
     {
-      if (reported)
-	out << ",\n" << indent << "its CRC (modversions) changed from ";
-      else
-	out << "\n" << indent << "CRC value (modversions) changed from ";
-
-      out << std::showbase << std::hex
+      out << indent << "CRC (modversions) changed from "
+	  << std::showbase << std::hex
 	  << symbol1->get_crc() << " to " << symbol2->get_crc()
-	  << std::noshowbase << std::dec;
-
-      reported = true;
+	  << std::noshowbase << std::dec
+	  << "\n";
     }
-
-  if (reported)
-    out << "\n";
-
-  return reported;
 }
 
 /// For a given symbol, emit a string made of its name and version.
diff --git a/src/abg-reporter-priv.h b/src/abg-reporter-priv.h
index 65786a0fa9c5..a7c4878c3279 100644
--- a/src/abg-reporter-priv.h
+++ b/src/abg-reporter-priv.h
@@ -206,7 +206,7 @@  maybe_report_diff_for_member(const decl_base_sptr&	decl1,
 			     ostream&			out,
 			     const string&		indent);
 
-bool
+void
 maybe_report_diff_for_symbol(const elf_symbol_sptr&	symbol1,
 			     const elf_symbol_sptr&	symbol2,
 			     const diff_context_sptr&	ctxt,
diff --git a/tests/data/test-abidiff-exit/test-crc-report.txt b/tests/data/test-abidiff-exit/test-crc-report.txt
index ddba41f40dad..2dbfa555b4ce 100644
--- a/tests/data/test-abidiff-exit/test-crc-report.txt
+++ b/tests/data/test-abidiff-exit/test-crc-report.txt
@@ -4,12 +4,10 @@  Variables changes summary: 0 Removed, 1 Changed, 0 Added variable
 1 function with some indirect sub-type change:
 
   [C] 'function void func1(E)' has some indirect sub-type changes:
-
-    CRC value (modversions) changed from 0x10000001 to 0x10000002
+    CRC (modversions) changed from 0x10000001 to 0x10000002
 
 1 Changed variable:
 
   [C] 'int var1' was changed:
-
-    CRC value (modversions) changed from 0x30000001 to 0x30000002
+    CRC (modversions) changed from 0x30000001 to 0x30000002
 
diff --git a/tests/data/test-abidiff/test-crc-report.txt b/tests/data/test-abidiff/test-crc-report.txt
index 4572a207a5ae..9bea309e2a2f 100644
--- a/tests/data/test-abidiff/test-crc-report.txt
+++ b/tests/data/test-abidiff/test-crc-report.txt
@@ -4,6 +4,5 @@  Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
 1 function with some indirect sub-type change:
 
   [C] 'function void exported_function()' has some indirect sub-type changes:
-
-    CRC value (modversions) changed from 0xe52d5bcf to 0xe52d5bd0
+    CRC (modversions) changed from 0xe52d5bcf to 0xe52d5bd0
 
diff --git a/tests/data/test-diff-filter/test-PR27569-report-0.txt b/tests/data/test-diff-filter/test-PR27569-report-0.txt
index 419c9fd4777c..99febd66601c 100644
--- a/tests/data/test-diff-filter/test-PR27569-report-0.txt
+++ b/tests/data/test-diff-filter/test-PR27569-report-0.txt
@@ -4,7 +4,6 @@  Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
 1 function with some indirect sub-type change:
 
   [C] 'function void device_add_disk(gendisk*)' at genhd.h:420:1 has some indirect sub-type changes:
-
-    CRC value (modversions) changed from 0x8afa957c to 0xa3bc03c
+    CRC (modversions) changed from 0x8afa957c to 0xa3bc03c
     parameter 2 of type 'int' was added