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

Fix test for integer division #1192

Merged
merged 3 commits into from Oct 26, 2022
Merged

Fix test for integer division #1192

merged 3 commits into from Oct 26, 2022

Conversation

d0cd
Copy link
Contributor

@d0cd d0cd commented Oct 20, 2022

This PR fixes the panic_on_halt condition for the integer division instruction.

@d0cd d0cd requested a review from howardwu October 20, 2022 04:07
@d0cd d0cd requested a review from ljedrz October 20, 2022 16:47
Copy link
Contributor

@ljedrz ljedrz left a comment

Choose a reason for hiding this comment

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

I'm not too familiar with this particular logic, but what I can recommend is to extend the test in question with an additional division operation that falls under these extended conditions.

@howardwu
Copy link
Member

@d0cd can we add an explicit test case to check for the failure case that was detected?

@d0cd d0cd requested a review from ljedrz October 24, 2022 18:51
Copy link
Contributor

@ljedrz ljedrz left a comment

Choose a reason for hiding this comment

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

Left 2 suggestions.

Copy link
Member

@howardwu howardwu left a comment

Choose a reason for hiding this comment

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

LGTM

d0cd and others added 3 commits October 25, 2022 17:54
Co-authored-by: ljedrz <ljedrz@users.noreply.github.com>
Signed-off-by: d0cd <pranavsaig@gmail.com>
@d0cd d0cd changed the base branch from testnet3 to testnet3.2 October 26, 2022 00:57
Copy link
Contributor

@ljedrz ljedrz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@howardwu howardwu merged commit 69744ba into testnet3.2 Oct 26, 2022
@howardwu howardwu deleted the fix/div-instruction-test branch October 26, 2022 22:47
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