[PATCH] D114224: Add JSONScopedPrinter class

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:619-620
+      for (const APSInt &Item : List) {
+        JOS.rawValueBegin() << Item;
+        JOS.rawValueEnd();
+      }
----------------
I'd pull this into a little helper function, shared by `printNumber`. It just helps avoid a (small) amount of duplication, but also helps label what the code does. (Ideally, I'd actually suggest enhancing the JSON output stream interface, but up to you whether you want to go that far)


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:96
 public:
-  ScopedPrinter(raw_ostream &OS) : OS(OS), IndentLevel(0) {}
+  enum ScopedPrinterKind {
+    Standard,
----------------
jhenderson wrote:
> All the new enums probably want to be `enum class`.
This is marked as done but I don't see it here?


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:36
 
+TEST_F(ScopedPrinterTest, GetKind) {
+  std::string StreamBuffer;
----------------
This and the classof test don't rely on the fixture, so I'd a) change them to not use the fixture, and b) move them above it. Alternatively, see my comment below.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:45
+  EXPECT_EQ(ScopedPrinter::ScopedPrinterKind::JSON, JSONWriter.getKind());
+  // JSONScopedPrinter fails an assert if nothings been printed.
+  JSONWriter.printString("");
----------------
Either "nothing is" or "nothing's". Same below.

You might want to consider enhancing the current fixture, as an alternative to making these two tests not use it. In this case, I'd add the `std::string`, `raw_string_ostream`, `ScopedPrinter` and `JSONScopedPrinter` local variables used here and in the verify* functions into the base class, and then use the `TearDown` method to ensure JSONScopedPrinter has that empty string written.

Aside: it seems to me that this assertion is bogus - it's not that unreasonable to create a printer, but write nothing to it, to get an empty output.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:1063
 
 TEST_F(ScopedPrinterTest, StartLine) {
   auto PrintFunc = [](raw_string_ostream &OS, ScopedPrinter &W) {
----------------
Jaysonyan wrote:
> 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.
Okay. I haven't looked into this, so I'll trust your judgement.


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

https://reviews.llvm.org/D114224