[PATCH] D114684: Add ScopedPrinter unit tests

[Jayson Yan via Phabricator via llvm-commits]

Jaysonyan added inline comments.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:51
+    {
+      DictScope D(W);
+      uint64_t Unsigned64Max = std::numeric_limits<uint64_t>::max();
----------------
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. 


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

https://reviews.llvm.org/D114684