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

Infer base for int #36

Merged
merged 1 commit into from
Feb 7, 2024
Merged

Infer base for int #36

merged 1 commit into from
Feb 7, 2024

Conversation

djboni
Copy link
Contributor

@djboni djboni commented Feb 1, 2024

Hexadecimals were being parsed as float.

Let parseInt() to infer the integer base by passing 0 as the third argument.

@@ -181,8 +181,7 @@ pub const Value = union(enum) {
const raw = tree.getRaw(node.start, node.end);

try_int: {
// TODO infer base for int
const int = std.fmt.parseInt(i64, raw, 10) catch break :try_int;
const int = std.fmt.parseInt(i64, raw, 0) catch break :try_int;
Copy link
Owner

Choose a reason for hiding this comment

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

Looks like a good first step. I am wondering though, and asking really since I don't see many YAML files day-to-day except for machine-generated dylib stubs on macOS: is it possible to have int encoded in different bases without a prefix in YAML?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They need a prefix in YAML 1.2 and in this implementation.

From the parseInt() documentation, it infers the base according to the prefix:

"When base [3rd argument] is zero the string prefix is examined to detect the true base:

  • A prefix of “0b” implies base=2,
  • A prefix of “0o” implies base=8,
  • A prefix of “0x” implies base=16,
  • Otherwise base=10 is assumed."

According to the spec, YAML parses integers in decimal, hexadecimal, and octal bases. However, it should not parse an hex/oct without the corresponding prefix.

One interesting bug/feature is that this implementation allows binary base (0b1101), which is not supported by YAML 1.2.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I know that std.fmt.parseInt uses the prefix to infer the base, hence why I was asking if this matches YAML spec. But it seems it does, so perfect then!

@kubkon kubkon merged commit 9308a64 into kubkon:main Feb 7, 2024
3 checks passed
@djboni djboni deleted the infer_base_for_int branch February 8, 2024 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants