[PATCH] D67236: [MC] Fix undefined behavior in MCInstPrinter::formatHex

[David Blaikie via Phabricator via llvm-commits]

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

Seems fairly reasonable to me - test might be able to be cleaned up a bit. Code might be cleaned up a bit (either now or in a follow-up commit)



================
Comment at: llvm/lib/MC/MCInstPrinter.cpp:88-93
+    if (Value == std::numeric_limits<int64_t>::min())
+      return format("-0x8000000000000000");
+    else if (Value < 0)
       return format("-0x%" PRIx64, -Value);
     else
       return format("0x%" PRIx64, Value);
----------------
There's a lot of else-after-return here, and maybe a special case might work here:

  if (Value < 0) {
    if (Value == min)
      return constant
    return format("-0x%"... -Value);
  }
  return format(...)

(this means only one test for the positive case, rather than 2)

But all that might be better as a separate cleanup patch.


================
Comment at: llvm/unittests/MC/MCInstPrinter.cpp:60-63
+Printer &getPrinter() {
+  static Printer P;
+  return P;
+}
----------------
Could probably put this in a test fixture, rather than a singleton? (but if there's nearby prior-art, I'm OK with it) (oh, the Printer class itself could be the test fixture class)


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

https://reviews.llvm.org/D67236