-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
support UInt & BigInt in TOML #47903
Conversation
Not sure if the test failure is due to this PR? The TOML tests pass locally for me |
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.
This is getting there, but I think if we're going to use native Julia types to represent integer literals, we should go all the way and use the same native types to represent integer literals as we would in Julia. In other words:
- Decimal literals from
-2^63:2^63-1
areInt64
- Decimal literals from
-2^127:2^127-1
areInt128
- Larger decimal literals are
BigInt
For binary, octal and hex literals, the type is determined by the number of digits the literal has (not the value of the literal, but the number of digits, d
, that it is written with):
- Binary literals are
UInt$n
wheren = bits(2, d)
- Octal literals are
UInt$n
wheren = bits(8, d)
- Hex literals are
UInt$n
wheren = bits(16, d)
Where bits(b, d) = 8*nextpow(2, d*log2(b)/8)
. Except that UInt$n
is replaced by BigInt
once n
is greater than 128 since we don't have unsigned types larger than that.
I am not sure the type you get back should be quite that closely dependent on the file content. I would think Int/UInt/BigInt should suffice and be cleaner for the API. It is not quite like parsing source code, where the semantic intent is being conveyed intentionally also by the code length. |
Yes, having a whole big range of types just feels like it would cause more compilation etc for very little value. Agree with Int/UInt/BigInt and that's it. |
Edit: I think the spec says Int64/UInt64, but yes |
Spec only says at least Int64. |
should I remove the support of I think the other reason we cannot support Uint8 etc. just that this will be quite breaking (they used to be returning |
Well, it would only be breaking for unsigned integer literals, which I suspect are not widely used in our world. The point of using different types is to use the type as a way to preserve more information about how to print values (i.e. how many digits they need). If we're going to to Int64/UInt64/BigInt and print UInt64 as hex, then I'm not sure we want to use Julia's printing for those values. If I round trip a TOML file with |
I also think that keeping the |
OK, so I kept the original parsing, where
for printing, signed integers are printed via I copy-pasted |
@StefanKarpinski bump, do you have any other comments? Can we merge this? |
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.
Looks good although I'm not seeing where this tests for printing of unsigned values.
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.
LGTM
Thanks for this @Roger-luo |
This reverts commit d61cfd2.
From a type-stability perspective, this restores a lot of our behavior before JuliaLang#47903. As it turns out, 10 of the 11 uses of `parse_int` introduced in that PR are unnecessary since the TOML format already requires the parsed value to be within a very limited range. Note that this change does not actually revert any functionality (in contrast to JuliaLang#49576)
From a type-stability perspective, this restores a lot of our behavior before #47903. As it turns out, 10 of the 11 uses of `parse_int` (now called `parse_integer`) introduced in that PR are unnecessary since the TOML format already requires the parsed value to be within a very limited range. Note that this change does not actually revert any functionality (in contrast to #49576)
From a type-stability perspective, this restores a lot of our behavior before #47903. As it turns out, 10 of the 11 uses of `parse_int` (now called `parse_integer`) introduced in that PR are unnecessary since the TOML format already requires the parsed value to be within a very limited range. Note that this change does not actually revert any functionality (in contrast to #49576) (cherry picked from commit 59c3c71)
This partially implements @StefanKarpinski's option 2 here: JuliaLang/TOML.jl#46 (comment), with the following convention
resolves JuliaLang/TOML.jl#46
cc: @KristofferC