[PATCH] D114224: Add JSONScopedPrinter class

[Jayson Yan via Phabricator via llvm-commits]

Jaysonyan added inline comments.


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:97
+  enum ScopedPrinterKind {
+    Standard,
+    JSON,
----------------
jhenderson wrote:
> I'd avoid the use of the name `Standard`, as it could be confused with the C++ standard. I'd use `Base`, `Plain` or `Basic` for the enum value.
Updated to use Base.


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:509
+  enum ScopeKind {
+    Standard,
+    Attribute,
----------------
jhenderson wrote:
> Similar comment to the above: don't use the name "Standard" for an enum value. Actually, in this case, without looking at the code, I have no clue what "Standard" even means. It should be renamed to reflect what it is used for.
Updated to `NoAttribute`.


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:521
+
+  SmallVector<ScopeContext, 16> ScopeHistory;
+  json::OStream JOS;
----------------
jhenderson wrote:
> At one point, there was some effort to set good defaults for SmallVector, allowing you to omit the second argument. It's less useful here, because we have a good guess about the amount of nesting. I think it's more likely to be a max of about 4 or 5, so 16 is probably way too high. Perhaps worth doing an audit of the usage within the LLVM code and add a few more points above the current maximum scoping (yes, I know JSONScopedPrinter isn't used really yet, but you can imagine how it would look, based on how ScopedPrinter is used already).
Looking through the usage of `ScopedPrinter` the most amount of nested scopes I came across was 6. This was a tie between a few methods in `llmv-readobj` (`LLVMElfDumper::printVersionDependencySection`, `LLVMElfDumper::printBBAddrMaps`, and `COFFDumper::printCodeViweSymbolSection`). Inside each of these methods there are 4 nested scopes and paired with the 2 scopes that would be added by `JSONScopedPrinter` (highest level `[]` and `{}` for each file) so I think the most nesting is 6. I'll update the `SmallVector` size to be 8.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:1063
 
 TEST_F(ScopedPrinterTest, StartLine) {
   auto PrintFunc = [](raw_string_ostream &OS, ScopedPrinter &W) {
----------------
jhenderson wrote:
> I think we need a JSONScopedPrinter version of this test, since we override `startLine` in that class.
My mistake, I actually mean to remove the overridden implementations of both `startLine` and `getOStream`. For the `JSONScopedPrinter` to provide these methods, it relies on `json::OStream::rawValueBegin()` which can only be used where values are used (elements of arrays or values to attributes). So if `startLine` or `getOStream` are called in any place which aren't these contexts (which is most of the time) then assertions inside `json::OStream` fail. So I think it might be more desirable to just rely on the `ScopedPrinter` implementation of both these methods.


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

https://reviews.llvm.org/D114224