-
Notifications
You must be signed in to change notification settings - Fork 95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Instruction debug fmt improvements #546
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I just left a couple nits.
I hadn't thought of just adding it to the Debug
. This seems like a good way to get what we wanted without touching much code.
fuel-asm/src/panic_instruction.rs
Outdated
Ok(instr) => write!(f, "{:?}", instr)?, | ||
Err(_) => write!(f, "Unknown")?, | ||
}; | ||
write!(f, " (bytes ")?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think I'd prefer bytes:
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, changed.
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
match Instruction::try_from(self.0) { | ||
Ok(instr) => write!(f, "{:?}", instr)?, | ||
Err(_) => write!(f, "Unknown")?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Maybe something like Unknown Instruction
or Unknown Operation
might be slightly more clear when the caller doesn't know what to expect in this field. I don't feel strongly about it though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It already says instruction: Unknown
, so I think it's good enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like both variants=D
Currently the
reason.instruction
sub-field ofPanicInstruction
is just a raw integer representation of the instruction bytes. This PR changes theDebug
implementation to decode the instruction when possible. It will also show the bytes in hexadecimal format, which is more useful for debugging as it can be used to spot the instruction in a hexdump, and it shows the opcode byte separately.This PR also changes the debug format of register (non-immediate) operands to be hexadecimal, as it is the established convention (e.g. the specification lists registers using hex values, and do does most of the test code).
Before
After