[PATCH] D114225: Add JSONScopedPrinter to llvm-readelf

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
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);
   }
----------------
Jaysonyan wrote:
> jhenderson wrote:
> > Jaysonyan wrote:
> > > jhenderson wrote:
> > > > 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?
> > > I agree that `--elf-output-style` shouldn't impact non-ELF formats. Although the work of adding a `JSONScopedPrinter` was not ELF-specific and it may be desirable to add json support for other formats in the future. This makes me wonder if `--elf-output-style` is an appropriate flag to enable json through or if it'd be preferable to add a format-agnostic flag (ex: `--output-format`). This could lend itself well if we choose to support json output of other obj formats in the future but could be confusing how `--elf-output-style` and `--output-format` would interact with each other. I'm not sure if this is something worth considering since there are currently no plans to support other obj formats but don't want to lock us in to an inconvenient flag choice for the future.
> > > 
> > > Otherwise, conceptually it makes sense to move this work into `ObjDumper` methods. One issue I see is that any usage of `DictScope` or `ListScope` relies on the ctor and dtor to output the opening/closing braces. So it may be difficult to just move this logic to other methods, especially if we have to initialize multiple `DictScopes`/`ListScopes` and just returning a `unique_ptr` would be insufficient. 
> > Yes, changing `--elf-output-style` to just `--output-style` was something I've considered a good idea for quite some time. This issue becomes more important as we add JSON support, but I think we can defer it to later on (it shouldn't block this work).
> > 
> > > One issue I see is that any usage of `DictScope` or `ListScope` relies on the ctor and dtor to output the opening/closing braces.
> > 
> > I'm a little lost which constructor we're talking about here? The `ObjDumper` constructor or the `DictScope`/`ListScope` constructors?
> Referring to the `DictScope`/`ListScope` ctor/dtor. Since the opening braces are printed in the constructor and the closing braces are printed in the destructor, moving any instantiations of `ListScope`/`DictScope` inside a `printFileSummary` method becomes difficult since we would still need `DictScope`/`ListScope` to get destructed at the end of the `dumpObject` method rather than at the end of the `printFileSummary` method.
Should the top-level `DictScope` be a member of the `JSONELFDumper`? Would that help solve this issue?

Related aside: perhaps in JSON mode, the file summary could become a separate object within the object for the file?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114225