[PATCH] D114684: Add ScopedPrinter unit tests
Jayson Yan via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 30 00:08:39 PST 2021
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
More information about the llvm-commits
mailing list