* [PATCH] abidiff: improve whitespace generation in symbol diff report
@ 2021-11-26 11:06 Matthias Maennich
2022-01-19 14:04 ` Dodji Seketeli
0 siblings, 1 reply; 2+ messages in thread
From: Matthias Maennich @ 2021-11-26 11:06 UTC (permalink / raw)
To: libabigail; +Cc: dodji, gprocida, kernel-team, maennich
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(-)
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
--
2.34.0.rc2.393.gf8c9666880-goog
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] abidiff: improve whitespace generation in symbol diff report
2021-11-26 11:06 [PATCH] abidiff: improve whitespace generation in symbol diff report Matthias Maennich
@ 2022-01-19 14:04 ` Dodji Seketeli
0 siblings, 0 replies; 2+ messages in thread
From: Dodji Seketeli @ 2022-01-19 14:04 UTC (permalink / raw)
To: Matthias Maennich; +Cc: libabigail, gprocida, kernel-team
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,
--
Dodji
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-01-19 14:04 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-26 11:06 [PATCH] abidiff: improve whitespace generation in symbol diff report Matthias Maennich
2022-01-19 14:04 ` 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).