[PATCH] D114224: Add JSONScopedPrinter class

[Jayson Yan via Phabricator via llvm-commits]

Jaysonyan added inline comments.


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


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:619-620
+      for (const APSInt &Item : List) {
+        JOS.rawValueBegin() << Item;
+        JOS.rawValueEnd();
+      }
----------------
jhenderson wrote:
> 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)
I've added a helper function for now. I don't imagine it would take too much to enhance the JSON class to handle APSInt but I don't have the capacity to investigate right now.


================
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("");
----------------
jhenderson wrote:
> 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.
Updated the test fixture to handle the teardown. I needed to add a check to ensure we're only printing an empty string if there hasn't been a call to `verifyJSONScopedPrinter` since `printString` can only be called under specific contexts. Alternatively I could call something like `DictScope(W,  "Label")` which can be called under all contexts (since it uses the history stack) but I held off because calling `printString("")` with the empty string felt more appropriate. Let me know if you have any opinions on this. 


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

https://reviews.llvm.org/D114224