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

Change String#to_i to parse octals with prefix 0o #7691

Merged

Conversation

@icy-arctic-fox
Copy link
Contributor

commented Apr 18, 2019

This is a breaking change.

Previously, numbers starting with 0 would be parsed as octals. Now that is not the case, and they will be parsed as base-10. The 0o prefix must be present to treat it as an octal. This addresses parsing 0 with prefix: true raising an error.

This PR should resolve #7689

Update:
The current behavior in Crystal can be preserved by adding leading_zero_is_octal: true to the #to_i calls. By default however, leading_zero_is_octal is false and expects 0o as a prefix. The YAML parser uses this so that both formats are recognized.

@icy-arctic-fox

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

Turns out this would also break YAML parsing of integers. The tests show that the YAML parser still behaves in the old way, where a 0 prefix means octal, instead of 0o. Working on a fix for that now.

@icy-arctic-fox icy-arctic-fox force-pushed the icy-arctic-fox:update-to-i-octal-prefix branch 2 times, most recently from 70be960 to fc5610f Apr 18, 2019

Change String#to_i to parse octals with prefix 0o
This is a breaking change.
Previously, numbers starting with 0 would be parsed as octals.
A previous change in Crystal requires octals start with 0o.
Now that is not the case, and they will be parsed as base-10.
The 0o prefix must be present to treat it as an octal.
This addresses parsing 0 with prefix: true raising an error.

Additionally, YAML parsing used the old style.
This updates the spec to handle the new parsing.
There is no official documentation for YAML (that I'm aware of) for octals.

@icy-arctic-fox icy-arctic-fox force-pushed the icy-arctic-fox:update-to-i-octal-prefix branch from fc5610f to f4fa716 Apr 18, 2019

@asterite

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

Maybe a leading_zero_is_octal = false argument is good enough (because some software, like YAML, might still need this behavior).

@icy-arctic-fox icy-arctic-fox marked this pull request as ready for review Apr 18, 2019

@icy-arctic-fox

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

Maybe a leading_zero_is_octal = false argument is good enough (because some software, like YAML, might still need this behavior).

I agree. There's a lot of places that still use a leading zero for octal notation. Linux permissions being one of them - 0755, 0644. Don't want to break that.

@icy-arctic-fox icy-arctic-fox force-pushed the icy-arctic-fox:update-to-i-octal-prefix branch 4 times, most recently from a0b0586 to ad699ba Apr 19, 2019

src/string.cr Outdated Show resolved Hide resolved

@icy-arctic-fox icy-arctic-fox force-pushed the icy-arctic-fox:update-to-i-octal-prefix branch 2 times, most recently from 3a544d4 to 528448e Apr 19, 2019

@asterite asterite added this to the 0.29.0 milestone May 2, 2019

else
if leading_zero_is_octal
base = 8
ptr -= 1

This comment has been minimized.

Copy link
@straight-shoota

straight-shoota May 2, 2019

Member

Why decrement the pointer?

This comment has been minimized.

Copy link
@icy-arctic-fox

icy-arctic-fox May 2, 2019

Author Contributor

After looking closer, I don't think it is needed. It was to cover the case where there's a leading zero, but no o right after it. It would handle strings like 0123 and 0abc (invalid). I think I put it in because there's no prefix character (like b or x), so it needed to back up.

This comment has been minimized.

Copy link
@icy-arctic-fox

icy-arctic-fox May 2, 2019

Author Contributor

Travis build 15307.1 failed when this was removed. Looks like it broke parsing of !!int 0 in YAML. I'll see if there's a way I can address this without decrementing the pointer, unless you're ok with putting it back in.

@straight-shoota

This comment has been minimized.

Copy link
Member

commented May 2, 2019

The YAML changes look fine.
According to YAML 1.2 Core Spec, octal numbers are prefixed by 0o. 0 prefix is not mentioned, but it's implicitly included in the regex for base 10 integers. But it's probably okay to interpret them as octal numbers. That would be in accordance with the tag:yaml.org,2002:int YAML 1.1 Integer type (which the Core Spec references) where an octal number is represented by a 0 prefix and 0o is not mentioned in the valid value spec.
So this is kind of contrdictory, but I guess it's fine to simply resolve both prefixes to octal number.

@icy-arctic-fox icy-arctic-fox force-pushed the icy-arctic-fox:update-to-i-octal-prefix branch from 528448e to 741d706 May 2, 2019

Add leading_zero_is_octal to String#to_i variations
This allows parsing of octals in strings that use just 0 as a prefix instead of 0o.
When this flag is true, the prior behavior for parsing octals is used.
YAML parsing has been updated to accept 0 and 0o prefixes for octals.

@icy-arctic-fox icy-arctic-fox force-pushed the icy-arctic-fox:update-to-i-octal-prefix branch from 741d706 to 4dabf9d May 2, 2019

@asterite

This comment has been minimized.

Copy link
Member

commented May 3, 2019

Sorry, I tried to fix the conflicts after merging another PR and I forgot some stuff (I think default values).

@icy-arctic-fox Do you have time to fix this? I think CI passed fine, it just had a hiccup in the previous run.

@icy-arctic-fox

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

Looks like it should be fixed. Somehow the merge dropped one default value.

@asterite asterite merged commit cae08eb into crystal-lang:master May 3, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@icy-arctic-fox icy-arctic-fox deleted the icy-arctic-fox:update-to-i-octal-prefix branch May 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.