[PATCH] D114684: Add ScopedPrinter unit tests

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

In D114684#3168977 <https://reviews.llvm.org/D114684#3168977>, @Esme wrote:

> Should unit tests be added for `to_hexString()`, `to_string()` and `enumToString()` (from D114840 <https://reviews.llvm.org/D114840>)? Although they are not part of the `ScopedPrinter` class but just in `ScopedPrinter.h`.

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.

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

Aside from that, looks good.


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

https://reviews.llvm.org/D114684