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.

Sign up for GitHub

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

Jump to bottom

Show all instruction arguments as signed numbers consistently #307

Merged
merged 1 commit into from
Jan 30, 2025
Merged

Show all instruction arguments as signed numbers consistently #307

merged 1 commit into from
Jan 30, 2025

Conversation

wkwiatek
Copy link
Collaborator

Closes #289

@netlify Netlify
Copy link

netlify bot commented Jan 23, 2025 •
edited
Loading

✅ Deploy Preview for pvm-debugger ready!

Name Link
🔨 Latest commit 7c6e94e
🔍 Latest deploy log https://app.netlify.com/sites/pvm-debugger/deploys/67927b174df6c200087b2a16
😎 Deploy Preview https://deploy-preview-307--pvm-debugger.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@wkwiatek wkwiatek force-pushed the instruction-arguments-signed branch from b89a850 to 7c6e94e Compare January 23, 2025 17:23
mateuszsikora
mateuszsikora requested changes Jan 24, 2025
View reviewed changes
Copy link
Contributor

@mateuszsikora mateuszsikora left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It does not work. Take a look at ADD_IMM_64 here:
Screenshot 2025-01-24 at 12 50 07

It should be -1. I think the problem is in valueToNumeralSystem function. There you have BigInt.asUintN(64, value).toString(16)

Actually in hex it does make sense to show unsigned 😅

@mateuszsikora
Copy link
Contributor

mateuszsikora commented Jan 24, 2025 •
edited
Loading

It does not work. Take a look at ADD_IMM_64 here: Screenshot 2025-01-24 at 12 50 07

It should be -1. I think the problem is in valueToNumeralSystem function. There you have BigInt.asUintN(64, value).toString(16)

Actually in hex it does make sense to show unsigned 😅

OK it works correctly, sorry for the previous comment. There is a few cases that I am not sure if we should show negative numbers. For example unsigned branch instructions:

https://graypaper.fluffylabs.dev/#/579bd12/262603267403

tomusdrw
tomusdrw approved these changes Jan 24, 2025
View reviewed changes
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

I think in DEC we should always be showing signed numbers (also in registers, I'll add separate issue for that), while HEX is always unsigned.

@tomusdrw tomusdrw mentioned this pull request Jan 24, 2025
Open
@wkwiatek wkwiatek merged commit 1345d05 into main Jan 30, 2025
6 checks passed
@wkwiatek wkwiatek deleted the instruction-arguments-signed branch January 30, 2025 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mateuszsikora mateuszsikora mateuszsikora approved these changes

@tomusdrw tomusdrw tomusdrw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Display negative numbers consistently
3 participants
@wkwiatek @mateuszsikora @tomusdrw