Use (U)Int128 literals if possible #5545
Conversation
| MIN = new(1) << 127 | ||
| MAX = ~MIN | ||
| MIN = -170141183460469231731687303715884105728_i128 | ||
| MAX = 170141183460469231731687303715884105727_i128 |
There was a problem hiding this comment.
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.
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.
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 |
|
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 |
straight-shoota
added
kind:refactor
pr:on-hold-do-not-merge
|
@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
TODOless.