[PATCH] D64679: Display codeview type record values in hex representation instead of decimal

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/include/llvm/MC/MCExpr.h:137
   int64_t Value;
+  bool PrintInHex = false;
 
----------------
I'm not sure we should add this in here. I added @pcc for a second opinion. Somehow I feel like the MCExpr class hierarchy shouldn't be concerned with syntactic details, and the printer should receive some kind of printing policy instead. However, I see this field in MCSymbolRefExpr:
  /// Specifies how the variant kind should be printed.
  const unsigned UseParensForSymbolVariant : 1;

So, clearly printing options are already held in MCExpr objects. I could go either way, so I figured I'd ask Peter.


================
Comment at: llvm/include/llvm/MC/MCExpr.h:157
 
+  bool UseHexFormat() const { return PrintInHex; }
+
----------------
Functions should start with lower case letters:
https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly


================
Comment at: llvm/include/llvm/MC/MCStreamer.h:631
+  /// in a MCExpr for constant integers & prints in Hex format for certain modes.
+  virtual void EmitIntValueInHex(uint64_t Value, unsigned Size) { EmitIntValue(Value, Size); }
+
----------------
I suggest running `git clang-format` to address this and other formatting issues.


================
Comment at: llvm/lib/MC/MCExpr.cpp:49
+    auto PrintInHex = cast<MCConstantExpr>(*this).UseHexFormat();
+    if(PrintInHex)
+      OS << "0x" << Twine::utohexstr(Value);
----------------
There should be a space between the `if` and `(`, clang-format will fix it


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64679