[PATCH] D111658: Add JSON output skeleton to llvm-readelf

[Jayson Yan via Phabricator via llvm-commits]

Jaysonyan added inline comments.


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:82
+  virtual ~ScopedPrinterBase() {}
+  ScopedPrinterKind getKind() const { return Kind; }
+
----------------
jhenderson wrote:
> Do you need this and the `classof` functions? As far as I can tell, they aren't used anywhere.
I believe this is needed to perform `dyn_cast`. I was following the guide here: https://llvm.org/docs/HowToSetUpLLVMStyleRTTI.html


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:103
+
+class ScopedPrinter : public ScopedPrinterBase {
+public:
----------------
jhenderson wrote:
> Slight naming quibble: at the moment, it's not clear why `ScopedPrinter` is not the base class, and why `JSONScopedPrinter` is a sibling class (shares same base class) and not a derived class of `ScopedPrinter`. I'd make `ScopedPrinter` the base class, and call this derived class something else. Perhaps `UnstructuredScopedPrinter` or `PlainScopedPrinter` (other ideas welcome)?
I agree, the naming feels a bit weird right now. I held off on renaming this class and making any changes to the interface since the `ScopedPrinter` is used outside of `llvm-readobj` and I didn't want this change to affect any other tool. Although if this is something that you think is fine to do, I'd be happy to change it. I think `PlainScopedPrinter` seems like an appropriate name. 


================
Comment at: llvm/tools/llvm-readobj/ObjDumper.cpp:136
 
 void ObjDumper::printSectionsAsString(const object::ObjectFile &Obj,
                                       ArrayRef<std::string> Sections) {
----------------
jhenderson wrote:
> For this and the similar printSectionsAsHex, I wonder if it would make sense to make this a virtual function that the JSONELFDumper implements differently, again to avoid the casting. Thoughts?
I was hesitant to change too much of the `ScopedPrinter` interface to cater too closely to the needs of `llvm-readobj` since its used elsewhere as well. 


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:336-346
+  if (opts::Output == opts::JSON) {
+    JSONScopedPrinter *JSONWriter = cast<JSONScopedPrinter>(WriterBase);
+    JSONWriter->objectBegin();
+    JSONWriter->attributeBegin(FileStr);
+    JSONWriter->objectBegin();
+  } else if (opts::Output == opts::LLVM || opts::InputFilenames.size() > 1 ||
+             A) {
----------------
jhenderson wrote:
> How about the following:
> 
> 1) Add a `printFileHeader()` function to the `ScopedPrinterBase` interface. For the `JSONScopedPrinter`, it does the objectBegin calls etc. For the `ScopedPrinter`, it does the LLVM/filename count checks (it might even do the later Format/Arch pritning too in this case).
> 2) Add a `endFile()` function which is a noop for `ScopedPrinter` but for the `JSONScopedPrinter` does the objectEnd calls etc.
> 
> This will avoid needing additional checks for `opts::JSON`, and helps hide some of the details a little further - ideally once you've created a `ScopedPrinterBase` instance, you should be able to avoid ever referencing the subclasses.
Similar to my feeling about adding a `printSectionAsString` and `printSectionAsHex` to the base class, I'm hesitant about adding methods to meet specific needs of `llvm-readobj`. 

I assumed it would be nice to keep `ScopedPrinter` somewhat primitive but I'm interested if you have any thoughts on this?


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

https://reviews.llvm.org/D111658