[PATCH] D114052: Add JSONScopedPrinter as a subclass to ScopedPrinter with history stack

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Some general high-level comments for now:

1. I think this patch would benefit from being split into a few separate patches:
  1. Virtualize the necessary parts of `ScopedPrinter`.
  2. Add the JSONScopedPrinter class.
  3. Add the llvm-readelf changes to make use of the `JSONScopedPrinter` class.
2. I think JSONScopedPrinter (and possibly ScopedPrinter too, if it doesn't already have them) could do with gtest unit tests. It should be fairly straightforward to write them, and would help give confidence in the methods, especially as we may not use all of them up-front. The tests would also help with documenting what the code actually does, which will aid in future reviewing.
3. We need to consider what will happen when dumping archives containing non-ELF objects. In such a case, it won't be possible to dump JSON output (for now) for them. Should we emit an error/warning in this case?
4. For the *Internal functions, I think *Impl tends to be a more common naming pattern I've seen (the non-impl versions do the common work, and then call the virtual impl versions, as you've done).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114052