[PATCH] D114225: Add JSONScopedPrinter to llvm-readelf

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Tip: add "Depends on DXXXXXX" to the patch description (but no need for it to be in the final commit message) to get Phabricator to auto-link this patch to the predecessor patch. I believe this helps the pre-merge checks, and also provides a nice little "Stack" view in the interface.



================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3263-3264
+  if (InputFilenames.size() > 1 || A) {
+    OS << '\n';
+    OS << "File: " << FileStr << '\n';
+  }
----------------
Old code used "\n" rather than '\n'. I think the former may be more common elsewhere in LLVM in general, although I'm not aware of any concrete coding standard.

Also, can't we use the existing `W` in the base class here, to keep the code identical to what it was before?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:7342
+  FileScope = std::make_unique<DictScope>(this->W, FileStr);
+  DictScope D(this->W, "FileSummary");
+  this->W.printString("File", FileStr);
----------------
Isn't this a behaviour change? Is it necesssary? If so, I'd defer it to a separate patch, if at all possible (or put it in a prerequisite patch), so that this patch is a pure NFC if a user doesn't specify `--elf-output-style=json`. I'm also slightly surprised I didn't see any test failures?

I'd suggest in this patch to call out to the base class version to do most of the work, although there's now a slight impedence to doing that in the form of the additional new line printed in one case but not the other, which I'd query.

The base class version was doing the JSON style version before, so I wonder if there's a good reason for this and the base class to diverge?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:712
 
+template <typename ELFT> class JSONELFDumper : public LLVMELFDumper<ELFT> {
+public:
----------------
Jaysonyan wrote:
> jhenderson wrote:
> > It's not clear to me why we inherit from `LLVMELFDumper` in this patch. This probably needs a comment at the very least.
> Added a comment! Let me know if you think it could provide more detail.
Thanks. Missing a trailing `.` though.

The limited nature of this new dumper class shows that we're not far off the ideal of having just one common class, which takes a ScopedPrinter (or JSONScopedPrinter), but I don't think we'll get there with this patch.


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:233
   if (Arg *A = Args.getLastArg(OPT_elf_output_style_EQ)) {
-    StringRef V(A->getValue());
-    if (V == "LLVM")
-      opts::Output = opts::OutputStyleTy::LLVM;
-    else if (V == "GNU")
-      opts::Output = opts::OutputStyleTy::GNU;
-    else
-      error("--elf-output-style value should be either 'LLVM' or 'GNU'");
+    opts::Output = llvm::StringSwitch<opts::OutputStyleTy>(A->getValue())
+                       .Case("LLVM", opts::OutputStyleTy::LLVM)
----------------



================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:233
   if (Arg *A = Args.getLastArg(OPT_elf_output_style_EQ)) {
-    StringRef V(A->getValue());
-    if (V == "LLVM")
-      opts::Output = opts::OutputStyleTy::LLVM;
-    else if (V == "GNU")
-      opts::Output = opts::OutputStyleTy::GNU;
-    else
-      error("--elf-output-style value should be either 'LLVM' or 'GNU'");
+    opts::Output = llvm::StringSwitch<opts::OutputStyleTy>(A->getValue())
+                       .Case("LLVM", opts::OutputStyleTy::LLVM)
----------------
jhenderson wrote:
> 
Perhaps pull `A->getValue()` into a helper variable, since you use it in the error message below.


================
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;
----------------
Jaysonyan wrote:
> jhenderson wrote:
> > Probably time to turn this into a `StringSwitch`.
> I needed to provide a default option to know when to send an error message so I added an `UNKOWN` entry to the `OutputStyelTy` enum. If this seems inappropriate I can also change to just using a `switch` statement.
`UNKNOWN` is a reasonable addition, in my opinion.


================
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'");
   }
----------------
jhenderson wrote:
> 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).
I think the text as it was was a little better, and I'd just add "but was \"foo\"" to the end.

Aside: general LLVM style for error messages is no leading capital letter.


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

https://reviews.llvm.org/D114225