Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add
U128
type #1600Add
U128
type #1600Changes from 33 commits
f3c2e53
974dde9
50e1188
16c118c
9d6796f
6435308
603f437
2437209
0cc9e93
bea85f6
136362f
c378550
ce5534d
1b70ec7
2a68d1d
680ad16
e0720b2
e64d83e
0d35b9d
ec7d94f
f2c34fa
93cd3b3
0a3c2a6
a6b6e7f
4f865bf
2844a1d
e91d917
82325e7
de6fd09
ec90475
d552adf
dc528df
bc25395
3aaa6fd
b4bd182
b087157
36f5ef7
3e29ad0
0602360
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Do these belong somewhere more general as
pub
definitions? Likenum
or somewhere?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.
hmm. My understanding was that the
flag
op had to be used in the same asm block as the op you're trying to allow overflow for, otherwise, the bit is unset after the block returns. Maybe a misunderstanding on my part, perhaps I was confused with$of
being cleared after an operation.Is this working?
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.
Also, perhaps rather than
movi r1 i3;
here you could initialize a second register with the value as a named variable (or even better, as aconst
) to:a.) help document the function better, and
b.) to follow the general guideline of "if it can be done outside of asm, do it outside of asm".
ie:
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.
And finally, it seems that the flag values for
enable
anddisable
are reversed.An unset flag (ie:
0
) should be the default state in which panic occurs on overflow.Setting to a non-zero value should enable overflow.
Edit: I think it's actually just the names that seem reversed to me. like,
disable_overflow
should bedisable_panic_on_overflow
, andenable_overflow
should beenable_panic_on_overflow
.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.
What about
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 think a combination of a descriptively-named register and a const value would be best, and in line with
tx.sw
and other modules where we use named const values to initialize registers with.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.
We had talked about a stdlib module for wrapping VM opcodes, this would probably fit the bill. Will look for the open issue.
#1074
Perhaps I'll just add a task to that issue to create a new module and extract these in a separate PR.
Done.
cc @otrho
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.
Fixed in 36f5ef7. Filed #1664 to follow-up on cleaner constants.
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.
@otrho given that no other part of the stdlib needs these functions, can we defer it to a future PR?