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

[Jayson Yan via Phabricator via llvm-commits]

Jaysonyan added a comment.

> 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.

Will do! Do we want to test each method in isolation? or possibly in conjunction with each other to test things like the history stack? or possibly both?

> 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?

I think emitting a warning to the error stream seems reasonable in this case. Since it may still be desirable to output the information for remaining ELF objects inside those archives.

> 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).

Thanks for the insight! Addressed in D114223 <https://reviews.llvm.org/D114223>.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114052