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

Mon Jul 15 15:50:22 PDT 2019

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

lgtm with the `explicit` marker removed. Thanks!

================
Comment at: llvm/include/llvm/MC/MCExpr.h:137
   int64_t Value;
+  bool PrintInHex = false;

----------------
nilanjana_basu wrote:
> pcc wrote:
> > rnk wrote:
> > > 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.
> > I guess MCExprs are sort of like clang expressions in that they are in fact syntactic, e.g. you can express the number 2 as an MCBinaryExpr 1+1 if you wanted. We could even preserve the number format given assembly input, although it's not clear how useful that would be. So I wouldn't be too opposed to storing the format here.
> I did it this way because the print function prints different types of MCExpr, which might have different syntactic needs. Changing the function parameter only for printing MCConstantExpr seemed awkward.
Alright, sounds good to me. It gives us the flexibility to print numbers contained in expressions in hex as well, so that's nice.

================
Comment at: llvm/include/llvm/MC/MCExpr.h:142

+  explicit MCConstantExpr(int64_t Value, bool PrintInHex)
+      : MCExpr(MCExpr::Constant, SMLoc()), Value(Value),
----------------
I believe `explicit` on a multi-argument constructor does nothing, so I would remove it. It's only to prevent excessive unintended automatic conversions.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64679