[PATCH] D137089: [llvm-readobj] Fix JSON output for Relocations

Wed Mar 1 00:54:33 PST 2023

jhenderson added a comment.

I think you may have misinterpreted my previous comments about testing a little. I don't think you need to test all the details that you are now testing in this patch. In particular, the error cases are from common code, and don't really need testing for JSON, I'd have thought (you may have one test for error behaviour, to show the error message is not part of the JSON format, if you want, I guess). It's probably sufficient to show that relocations are printed in "expanded" form, whether or not --expand-relocs is used for JSON output. Up to you, but I'm not convinced you even need to test the 32-bit versus 64-bit differences.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/relocations.test:12

+
 #      LLVM-64:Relocations [
----------------
Nit: please don't add this extra blank line.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/relocations.test:114

+# Show that JSON relocations are always expanded
+# RUN: llvm-readobj -r %t64 --elf-output-style=JSON --pretty-print \
----------------

================
Comment at: llvm/test/tools/llvm-readobj/ELF/relocations.test:425
+
+
 ## Case B: check the case when relocations can't be read from an SHT_RELA section.
----------------
Nit: only one blank line please.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/relocations.test:458-460

+
+
----------------
Nit: only one blank line please.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/relocations.test:691
+
+
 # RUN: llvm-readelf -r %t32 \
----------------
Ditto.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/relocations.test:719

+## Show that JSON outup always expands relocations
+# RUN: llvm-readobj -r --expand-relocs %t32 --elf-output-style=JSON --pretty-print\
----------------

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137089/new/

https://reviews.llvm.org/D137089