[PATCH] D114684: Add ScopedPrinter unit tests

[James Henderson via Phabricator via llvm-commits]

jhenderson 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:
> 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.


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

https://reviews.llvm.org/D114684