[PATCH] D114224: Add JSONScopedPrinter class

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Support/ScopedPrinter.cpp:50
+    : ScopedPrinter(OS, ScopedPrinter::ScopedPrinterKind::JSON),
+      JOS(OS, PrettyPrint ? 2 : 0), OuterScope(std::move(OuterScope)) {
+  if (this->OuterScope)
----------------
Consider adding a comment, as suggested inline, to "name" the pretty print/indentation parameter. The name should match the parameter's name.


================
Comment at: llvm/lib/Support/ScopedPrinter.cpp:52
+  if (this->OuterScope)
+    this->OuterScope.get()->setPrinter(*this);
+}
----------------
I don't think you need the `.get()`?


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:52
+    JSONScopedPrinter NoScopeWriter(OS, false);
+    NoScopeWriter.printString("NoScope");
+  }
----------------
For symmetry, I might be inclined to print a string in each of the three cases, not just the "no scope" case.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:67
+  ScopedPrinterTest()
+      : OS(StreamBuffer), Writer(OS), JSONWriter(OS, true),
+        HasPrintedToJSON(false) {}
----------------
I'd add a comment as suggested inline, to explain the boolean.


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

https://reviews.llvm.org/D114224