[PATCH] D114224: Add JSONScopedPrinter class

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:533-534
+public:
+  JSONScopedPrinter(raw_ostream &OS, int IndentSize = 0,
+                    Scope OuterScopeKind = Scope::None)
+      : ScopedPrinter(OS, ScopedPrinter::ScopedPrinterKind::JSON),
----------------
I'm not a fan of having to have an enum for the scoping kind, having looked at this. I'd much rather a class that does RAII-style management of the printer be passed in and ownership taken by this printer. I was going to suggest the following interface, but I see that the DelimitedScope takes a ScopedPrinter, leading to a chicken and egg problem.

```
JSONScopedPrinter(raw_ostream &OS, bool PrettyPrint = false, std::unique_ptr<DelimitedScope> &&Scope = std::unique_ptr<DelimitedScope>());
```
The first argument is unchanged. The second argument means control of the details of indentation is left to the class, rather than needing to know what "2" or "0" means (especially as "0" is special, as it means no new lines either). The third argument allows a user to provide the appropriate scoping mechanism for their use-case, but leaves the class to take and manage ownership; if it's defaulted, then no scoping will be used.

NB: the functionality of the second and third arguments would need testing (both default and explicit values), if they aren't already.

I wonder if the way to resolve this circular issue, is to have the DelimitedScope subclasses have a default constructor, which delays assigning the ScopedPrinter to a separate function. The scope opening would then be performed when the class is assigned its printer (and closing would only happen if a printer has been assigned). What do you think of this?


================
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("");
----------------
Jaysonyan wrote:
> 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. 
The only other idea I had was to make `JSONScopedPrinter` an `Optional` in the fixture, initialised in the corresponding verify method (and optionally in other tests, if needed), but I don't think that works.

I'm happy going with your suggestion.


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

https://reviews.llvm.org/D114224