Skip to content
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

Debugger fix #32

Merged
merged 6 commits into from
Jul 12, 2023
Merged

Debugger fix #32

merged 6 commits into from
Jul 12, 2023

Conversation

KGrykiel
Copy link
Collaborator

@KGrykiel KGrykiel commented Jul 7, 2023

fixed an issue where debugger had an error when trying to dump registers. It tried to use get() method instead of get_f() method. Also changed format of floating point registers to show up as floats instead of Hex.

@superlopuh
Copy link
Collaborator

@KGrykiel can you please assign reviewers and yourself as the assignee? I have a feeling @AntonLydike doesn't automatically gets a notification for PRs, but it's good practice in general to make it more likely that the people you want look at your GitHub PRs :)

examples/fibs.asm Outdated Show resolved Hide resolved
@@ -147,7 +147,10 @@ def dump_reg_a(self):
)

def _reg_repr(self, reg: str, name_len=4, fmt="08X"):
txt = "{:{}}=0x{:{}}".format(reg, name_len, self.get(reg, False), fmt)
if(reg in self.float_regs):
txt = "{:{}}={: .3e}".format(reg, name_len, self.get_f(reg, False))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

removed the debugger activation
@superlopuh
Copy link
Collaborator

Can you please run black .? It's not set up as a precommit hook in this repo.

Copy link
Owner

@AntonLydike AntonLydike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, will give it a quick test! Just a minor thing, maybe we can make this giant if statement a bit more readable using some variables or something? I can't really make sense of it right now...

Comment on lines 162 to 168
if (
reg in self.valid_regs
and self.get(reg, False) == 0
and reg not in Registers.named_registers()
or reg in self.float_regs
and self.get_f(reg, False)
):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What in tarnation? Can we make this a bit more readable?

Copy link
Collaborator Author

@KGrykiel KGrykiel Jul 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What in tarnation? Can we make this a bit more readable?

I had to create a seperate formatting condition for changing empty float registers into the gray colour so that it also uses get_f instead of get. It started out as two seperate if statements, one for regular registers (reg in self.valid_regs and self.get(reg, False) == 0 and reg not in Registers.named_registers()) and one for float registers (reg in self.float_regs and self.get_f(reg, False)), but I decided to combine them with an or since they led to the same result. Could split them up again perhaps?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you can also do something like this?

should_color_float = reg in self.float_regs and self.get_f(reg, False) != 0
should_color_int = reg in self.valid_regs and (
    self.get(reg, False) != 0 
    or reg in Registers.named_registers()
)

if should_color_float or should_color_int:
    # print colored
else:
    # print grey

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ye, sounds good as well, I'm on it

@AntonLydike AntonLydike merged commit 3b89387 into master Jul 12, 2023
4 of 5 checks passed
@superlopuh superlopuh deleted the debugger-fix branch July 12, 2023 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants