[PATCH] D114684: Add ScopedPrinter unit tests

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:26
+
+  void verifyScopedPrinter(StringRef Expected, PrintFunc Func) {
+    Func(Writer);
----------------
I'm liking the simplicity we're heading towards here, with this function and the members. As a next step, how about you add a "verifyAll" function which takes the expected strings (one for each type) and then calls the corresponding `verify*` function. This has the advantage of removing additional function calls from each test. The tests then just need to define the two strings and call the "verifyAll" function.

You could even go as far as have the fixture store StringRef members (default to empty) for `ExpectedScopedPrinter`, `ExpectedJSON` etc, and have the tests set those, rather than pass them into the function, although I'm not sure there's a huge amount of benefit to that.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:39
+    W.printString("|");
+    W.flush();
+  };
----------------
I'm in two minds about this following suggestion, so feel free to take it or leave it.

`W.flush()` is in every single lambda that I bothered checking. If we moved it to the verify* function(s), we'd avoid the need for it to be repeated here. The downside is of course that both verify functions will need it, but I think that's better than the repetition. What do you think?


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:51
+    {
+      DictScope D(W);
+      uint64_t Unsigned64Max = std::numeric_limits<uint64_t>::max();
----------------
Jaysonyan wrote:
> jhenderson wrote:
> > jhenderson wrote:
> > > jhenderson wrote:
> > > > Jaysonyan wrote:
> > > > > jhenderson wrote:
> > > > > > What's the reasoning for the `DictScope`? I don't see anything in the code that requires it, and it just confuses the test a bit.
> > > > > It's not required for the `ScopedPrinter` but it is for `JSONScopedPrinter`. It's possible to remove but it would just mean that we wouldn't be able to reuse this lambda for the `JSONScopedPrinter` equivalent test.
> > > > Okay, leave as-is.
> > > Actually, revisiting this, now that I've looked at the other related patches again. Is the only reason we need it because we don't add it within the JSONScopedPrinter constructor? If we added it there, could we omit it from here? See also my comments in D114225.
> > Did this comment get missed?
> Sorry, meant to wait to arrive at a conclusion on the discussion in D114225. I realized the ctor solution (while useful for D114225) isn't perfect for testing. Since at the time that we check the output the `JSONScopedPrinter` won't have gone out of scope yet so the `ListScope/DictScope` it's holding on to won't have outputted the closing `}/]`.
> 
> As an alternative solution I've added the `DictScope` inside `verifyJSONScopedPrinter` so that the `DictScope` goes out of scope by the time we check the output and so I've removed the unneeded calls to `DictScope` inside the lambdas. 
No worries. I made this comment before seeing your comments elsewhere.


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

https://reviews.llvm.org/D114684