-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Use (U)Int128 literals if possible #5545
base: master
Are you sure you want to change the base?
Conversation
MIN = new(1) << 127 | ||
MAX = ~MIN | ||
MIN = -170141183460469231731687303715884105728_i128 | ||
MAX = 170141183460469231731687303715884105727_i128 |
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.
Would be easy to have a character off here. Is there a spec asserting these numbers are 2**127 - 1
and -(2**127)
?
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.
wouldn't it be better if those numbers are calculated from macros directly, sth like:
MIN = {{ -(2 ** 127) }}_i128
MAX = {{ 2 ** 127 - 1 }}_i128
?
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.
Quite possible, these numbers were taken from rust's 128-bit integer support RFC - if they indeed turn out to be wrong, linked wiki page could use an update too.
MIN = new 0 | ||
MAX = ~MIN | ||
MIN = 0_u128 | ||
MAX = 340282366920938463463374607431768211455_u128 |
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.
ditto.
Oh, wait. Please don't merge this. The parser can't parse such big numbers yet. |
Should a literal like 17_000_000_000__000_000_000__000_000_000__000_000_000_i128 be parsed as |
@mmKALLL I don't see why not. I'm pretty sure the compiler completely ignores underscores inside of Number literals |
I think this might break some things especially on 32bit systems but I would like to see more steps like this. Here is a link to an issue with 3 PRs linking in the description. |
Is this GTG? IIUC failing CI checks are related to old Crystal version used in this jobs (pre i128 support). |
We can't merge it as long as CI is failing. This change breaks compatibility with Crystal 1.0. So we must either add guards for the Crystal version or postpone this for 2.0. Considering the triviality, I think it's probably fine to just wait. |
Good point. Let's w8 for 2.0 then 👍 |
I converted this to a draft despite being technically ready to be merged. We won't merge it anytime soon (because 2.0 is not on the horizon). The draft status is essentially similar to the |
One
TODO
less.