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


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:278-279
+      ListScope L(W, "StringList");
+      W.printString(StringRefValue);
+      W.printString(StringRefValue);
+    }
----------------
jhenderson wrote:
> The fact that these two cases are inside the ListScope makes the ListScope look like it's important to the behaviour here, but I don't think it is?
> 
> Also, why print twice?
Similar to the outside `DictScope` it is needed produce valid json when testing the `JSONScopedPrinter`. It's not really needed for testing `ScopedPrinter` but it allows us to reuse the lambda for both `ScopedPrinter` and `JSONScopedPrinter`.

I had added a second print since I thought it made sense to have multiple elements in a list but I realized that's not necessary. 


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

https://reviews.llvm.org/D114684