[PATCH] D114224: Add JSONScopedPrinter class

[Jayson Yan via Phabricator via llvm-commits]

Jaysonyan 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),
----------------
jhenderson wrote:
> 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?
I agree that constructing the `JSONScopedPrinter` with an enum is less preferable than constructing with a `DelimitedScope`. I've implemented your suggestion to allow for `DelimitedScope` subclasses to be default constructed and later add the `ScopedPrinter`. This allowed us to construct `JSONScopedPrinter` with a `DelimitedScope` rather than using an enum. 

Making this change also pointed out a weird piece of code which accesses the `ScopedPrinter` through the `DelimitedScope` member but it had access to the original ScopedPrinter. So I've updated that piece of code to directly access the ScopedPrinter. 

I've also added tests for pretty printing and this delimitedScope ctor param. Since they require specific ctor calls they don't use the test fixture. 


================
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:
> 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.
I'll leave it as is for now, possibly in the future that assertion will be removed and this extra work can just be deleted.


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

https://reviews.llvm.org/D114224