[PATCH] D114684: Add ScopedPrinter unit tests
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Dec 3 01:21:34 PST 2021
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
More information about the llvm-commits
mailing list