[PATCH] D114225: Add JSONScopedPrinter to llvm-readelf

[Jayson Yan via Phabricator via llvm-commits]

Jaysonyan added inline comments.


================
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);
----------------
jhenderson wrote:
> 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?
Sorry, I'm not sure if I fully understand what constitutes as a behaviour change. This method would only be run when the user has specified `--elf-output-style=JSON` (which is introduced in this patch) so it shouldn't impact any existing behaviour.

The base class version calls `W.startLine()` which isn't well supported with `JSONScopedPrinter` and my expectation was to override any methods in the base class which use `startLine()`. Beyond that, the `FileScope` is needed in this implementation based on our previous discussion (D114225#inline-1092086) to achieve the desired JSON output discussed in the initial patch (D111658#3072213). If we relied solely on the base class, we wouldn't get each file in their own `{}`.

In my opinion, it makes sense to not break this change out into its own patch because it relies on the existence of a `JSONELFDumper` which is introduced in this patch and this patch relies on this change so that the output is in a reasonable format (each file has it's own object). 


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

https://reviews.llvm.org/D114225