Page MenuHomePhabricator

Display codeview type record values in hex representation instead of decimal
AcceptedPublic

Authored by nilanjana_basu on Jul 12 2019, 4:20 PM.

Details

Reviewers
rnk
pcc
Summary

Code view type record "integer" type values are currently emitted in decimal format. This change will display these values in hex format for better readability.

Diff Detail

Repository
rL LLVM

Event Timeline

nilanjana_basu created this revision.Jul 12 2019, 4:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2019, 4:21 PM
rnk added a reviewer: pcc.Jul 12 2019, 5:10 PM
rnk added inline comments.Jul 12 2019, 5:18 PM
llvm/include/llvm/MC/MCExpr.h
137

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.

159
llvm/include/llvm/MC/MCStreamer.h
631

I suggest running git clang-format to address this and other formatting issues.

llvm/lib/MC/MCExpr.cpp
49

There should be a space between the if and (, clang-format will fix it

Used git clang-format to fix formatting issues

pcc added inline comments.Jul 12 2019, 5:41 PM
llvm/include/llvm/MC/MCExpr.h
137

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.

Cosmetic change of fixing function name to start with lower case

nilanjana_basu marked 5 inline comments as done.Jul 15 2019, 11:09 AM
nilanjana_basu added inline comments.
llvm/include/llvm/MC/MCExpr.h
137

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.

159

Fixed

nilanjana_basu marked 2 inline comments as done.Jul 15 2019, 11:10 AM
rnk accepted this revision.Jul 15 2019, 3:50 PM

lgtm with the explicit marker removed. Thanks!

llvm/include/llvm/MC/MCExpr.h
137

Alright, sounds good to me. It gives us the flexibility to print numbers contained in expressions in hex as well, so that's nice.

142

I believe explicit on a multi-argument constructor does nothing, so I would remove it. It's only to prevent excessive unintended automatic conversions.

This revision is now accepted and ready to land.Jul 15 2019, 3:50 PM