[PATCH] D114684: Add ScopedPrinter unit tests

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D114684#3170953 <https://reviews.llvm.org/D114684#3170953>, @Jaysonyan wrote:

>> Yes, they should be added, but I'd be inclined to say that they should be in a separate patch, allowing us to focus this on the ScopedPrinter class specifically.
>
> I can add these in a separate patch then.
>
>> @Jaysonyan - I spotted a unit test failure in the pre-merge checks above. You should take a look and address it. I think it's a classic case of a compiler treating a uint8_t/int8_t as char variants, and printing them as such. At a guess, this is a bug in the main code that the test is highlighting, rather than a bug in your change though. Is this bug present with the virtualization patch? In theory, this patch should be standalone, so I think a separate precursor small bug fix would be beneficial, so that this unit test patch can be submitted immediately.
>
> This was working locally and I believe that this might because I didn't mention that this diff depends on the virtualization diff. I've added that to the description and hopefully the failing test will fix itself. That being said, I think this indicates that the virtualization patch (D114223 <https://reviews.llvm.org/D114223>) includes a functional change (calling printList with `uint_8`/`int8_t`). Although I believe this would be closer to fixing an unrealized bug.

Agreed that this was probably a bug-fix. Might be worth highlighting this in a comment in the patch description.



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


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

https://reviews.llvm.org/D114684