[PATCH] D114684: Add ScopedPrinter unit tests

Fri Dec 3 15:20:00 PST 2021

Jaysonyan added a comment.

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

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

https://reviews.llvm.org/D114684