[PATCH] D114225: Add JSONScopedPrinter to llvm-readelf

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:712
 
+template <typename ELFT> class JSONELFDumper : public LLVMELFDumper<ELFT> {
+public:
----------------
It's not clear to me why we inherit from `LLVMELFDumper` in this patch. This probably needs a comment at the very least.


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:234-239
     if (V == "LLVM")
       opts::Output = opts::OutputStyleTy::LLVM;
     else if (V == "GNU")
       opts::Output = opts::OutputStyleTy::GNU;
+    else if (V == "JSON")
+      opts::Output = opts::OutputStyleTy::JSON;
----------------
Probably time to turn this into a `StringSwitch`.


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:241-242
     else
-      error("--elf-output-style value should be either 'LLVM' or 'GNU'");
+      error(
+          "--elf-output-style value should be either 'LLVM', 'GNU', or 'JSON'");
   }
----------------
It would be great if this message included what the actual value specified was, and now's a good time to change it whilst you're here. Providing more context is useful, because it may not always be obvious what the problem is. For example, a command-line of "llvm-readobj --elf-output-syle="GNU my/input/file.o", resulting from a typo, would not always be immediately obvious where the issue is (especially if this was in a script and the input file was in a variable etc).


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:340-347
+  if (opts::Output == opts::JSON) {
+    D = std::make_unique<DictScope>(Writer, FileStr);
+  } else if (opts::Output == opts::LLVM || opts::InputFilenames.size() > 1 ||
+             A) {
     Writer.startLine() << "\n";
     Writer.printString("File", FileStr);
   }
----------------
These two sets of if statements are actually in the wrong place, in my opinion: I think they should be behind interfaces in the ObjDumper and sub-classes, as appropriate, something like `printFileSummary` (which could take care of both).

I think it may be a slightly separate issue though. Here's my thinking:
# LLVM style is controlled via the --elf-output-style option, which has different defaults for llvm-readelf and llvm-readobj.
# The name of that option implies it has no impact for non-ELF formats, but this area of code is format-agnostic.
# JSON output probably actually wants to print Format/Arch/AddressSize.
# Everything else that is impacted by the --elf-output-style option is driven from the inheritance hierarchy. It seems odd that we have something that isn't.

At the very least, we could move the logic into virtual functions in ObjDumper, and then have JSON format provide its own behaviour. This would be a step in the right direction at least. What do you think?


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:561-563
+  if (opts::Output == opts::JSON) {
+    return std::make_unique<JSONScopedPrinter>(fouts());
+  }
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114225