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

Remove erroneous "Notes" from SUB and SUBI instructions #613

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

ironcev
Copy link
Member

@ironcev ironcev commented Oct 17, 2024

Description

This PR fixes the documentation of the SUB and SUBI instructions and makes it follow the same pattern as the ADD and ADDI instructions.

The documentation was wrongly using the term "overflow" in the "Notes" of those two instructions, instead of "underflow" (as used in the longer explanation below the instruction).

The PR simply removes the erroneous description from the "Notes" and leaves them empty, as in ADD and ADDI.

Before requesting review

  • I have reviewed the code myself

@ironcev ironcev self-assigned this Oct 17, 2024
@ironcev ironcev added documentation Improvements or additions to documentation comp:FVM Component: FuelVM labels Oct 17, 2024
@ironcev ironcev requested a review from a team October 17, 2024 15:09
@xgreenx xgreenx requested a review from Dentosal October 21, 2024 16:41
@Dentosal
Copy link
Member

Technically "integer underflow" is still an overflow, i.e. operation resulting a value outside the target domain. That word is widely used to describe this situation, for example in the llvm docs The note is not incorrect. That said, if it's unclear to you it could be improved.

Copy link
Member

@Dentosal Dentosal left a comment

Choose a reason for hiding this comment

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

Parity with add/addi is a good thing.

@ironcev ironcev merged commit f9e6b9e into master Oct 23, 2024
6 checks passed
@ironcev ironcev deleted the ironcev/fix-docs-for-sub-instructions branch October 23, 2024 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:FVM Component: FuelVM documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants