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: i257 negative zeros #262

Merged
merged 9 commits into from
Jan 26, 2024

Conversation

delaaxe
Copy link
Contributor

@delaaxe delaaxe commented Jan 25, 2024

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this introduce a breaking change?

  • Yes
  • No

Other information

@delaaxe delaaxe requested a review from 0xLucqs as a code owner January 25, 2024 10:56
Copy link
Collaborator

@0xLucqs 0xLucqs 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 that for clarity in error messages you shouldn't use the regular Add trait of u256 but rather this and use .expect("useful error message") on the result

@delaaxe
Copy link
Contributor Author

delaaxe commented Jan 25, 2024

I think that for clarity in error messages you shouldn't use the regular Add trait of u256 but rather this and use .expect("useful error message") on the result

done, i think you meant those functions

@0xLucqs
Copy link
Collaborator

0xLucqs commented Jan 25, 2024

No i was referring to the functions i linked (u256_overflowing_add,u256_overflowing_sub etc)

@delaaxe
Copy link
Contributor Author

delaaxe commented Jan 25, 2024

those are clumsier to use, the checked ones allow using expect more directly. unless i completely misunderstood what you wanted to do?

@0xLucqs
Copy link
Collaborator

0xLucqs commented Jan 25, 2024

checked_add calls overflowing_add so you get a small improvement. It shouldn't be hard to use just do

let (r, overflow) = u256_overflowing_add(lhs, rhs);
let r = r.expect("....");

@delaaxe
Copy link
Contributor Author

delaaxe commented Jan 25, 2024

the first returned value r is a u256, so there's no expect method on it. why reimplement a function that already exists (the checked one I used)?

i also don't want the overflow value

@delaaxe
Copy link
Contributor Author

delaaxe commented Jan 26, 2024

@LucasLvy can you check please?

@0xLucqs
Copy link
Collaborator

0xLucqs commented Jan 26, 2024

correct leave that as it is

@0xLucqs 0xLucqs merged commit f48961d into keep-starknet-strange:main Jan 26, 2024
3 checks passed
@delaaxe
Copy link
Contributor Author

delaaxe commented Jan 26, 2024

Thanks boss

@github-actions github-actions bot locked and limited conversation to collaborators Jan 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants