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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

To collate the common topics into one place, regarding additional virtual functions etc. I didn't register that `ScopedPrinter` is used outside the llvm-readobj code. Given that it is, I agree that modifying the base class to provide additional virtual functions would be undesirable. However, I did have another idea. Given that we ideally want a ScopedPrinter that has an interface suitable for llvm-readobj, how about we do the following:

1. Leave the original `ScopedPrinter` largely untouched, except making the common methods like `printNumber` virtual, adding a virtual destructor etc.
2. Within the llvm-readobj tree, create a new `ReadobjScopedPrinter` which is just an extension of the former, but with the additional methods added needed by llvm-readobj. This could be pure virtual, or provide default implementations of the extension functions.
3. Change all usages of `ScopedPrinter` to `ReadobjScopedPrinter` within the llvm-readobj code.
4. Change `JSONScopedPrinter` to derive from `ReadobjScopedPrinter` and implement the additional functions (and move it into the llvm-readobj code).

I'm not 100% happy with this last point. I feel like there's something in the Design Patterns space that would allow us to keep the JSON scoped printer generic, and provide a specialisation of it within llvm-readobj, but I haven't figured out how you'd get the llvm-readobj specific bit of the interface to work in this case yet.



================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:82
+  virtual ~ScopedPrinterBase() {}
+  ScopedPrinterKind getKind() const { return Kind; }
+
----------------
Jaysonyan wrote:
> 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
Yes, you're right: I meant to come back and delete this comment after seeing the casting later on.


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

https://reviews.llvm.org/D111658