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

SI-9015 Reject 0x and minor parser cleanup #4182

Merged
merged 1 commit into from Dec 9, 2014

Conversation

som-snytt
Copy link
Contributor

There was no negative test for what constitutes a legal literal.

The ultimate goal is for the test to report all errors in
one compilation.

This commit follows up the removal of "1." syntax to simplify
number parsing. It removes previous paulp code to contain the
erstwhile complexity.

Leading zero is not immediately put to the buffer. Instead,
the empty buffer is handled on evaluation. In particular, an
empty buffer due to 0x is a syntax error.

The message for obsolete octal syntax is nuanced and deferred
until evaluation by the parser, which is slightly simpler to
reason about.

@lrytz
Copy link
Member

lrytz commented Dec 3, 2014

I see that val a = 0x should be rejected according to the spec. But given that it's currently accepted, I'm not sure we should change that in a . release. How about a warning in 2.11.5, and an error in 2.12?

@som-snytt
Copy link
Contributor Author

@lrytz Showing that it's OK to fix bugs:

scala/scala-xml#36 (comment)

Clients might come to rely on certain behaviors, but that doesn't mean buggy behavior requires deprecation. Especially bugs that are plain wrong and not useful.

However, I'd be willing to take 0x syntax to scala-debate. 0FFFFx instead of 0xFFFF. Is that more bizarre than 3d? You don't have to know the base after the second char. And it looks might like math notation with the radix as trailing subscript.

@retronym
Copy link
Member

retronym commented Dec 4, 2014

I'd vote to just go ahead fix this one without a deprecation.

@lrytz
Copy link
Member

lrytz commented Dec 4, 2014

LGTM, really a great cleanup!

I'm more than happy be outvoted and skip deprecation of 0x == 0, so /cc @adriaanm.

@som-snytt
Copy link
Contributor Author

Since this is still on the queue, I improved a code comment.

FTR, while I was ironing my Scout uniform, I would also have improved the legacy param name on that other PR, if it were still active. /cc @lrytz

@adriaanm
Copy link
Contributor

adriaanm commented Dec 5, 2014

Pretty hexing stuff... Who writes 0x???

My only concern with this PR is that an unhexpecting user might not understand what it says on the tin, even though it's clear what's inside.

Could you reformulate more hexplicitly? Perhaps something like "SI-9015 reject 0x as illegal synthax`

@adriaanm
Copy link
Contributor

adriaanm commented Dec 5, 2014

(While you're at it, squash to 0x1 commit?)

@som-snytt
Copy link
Contributor Author

Sure, I can freebase as well as the next guy. I was just trying not to stress Jenkins and whoever got sucked into reviewering. Thanks to the reviewerers, BTW.

Only print error results.  Show deprecated forms.

Test for rejected literals and clean up parser

There was no negative test for what constitutes a legal literal.

The ultimate goal is for the test to report all errors in
one compilation.

This commit follows up the removal of "1." syntax to simplify
number parsing.  It removes previous paulp code to contain the
erstwhile complexity.

Leading zero is not immediately put to the buffer.  Instead,
the empty buffer is handled on evaluation.  In particular, an
empty buffer due to `0x` is a syntax error.

The message for obsolete octal syntax is nuanced and deferred
until evaluation by the parser, which is slightly simpler to
reason about.

Improve comment on usage of base

The slice-and-dicey usage of base deserves a better
comment. The difference is that `intVal` sees an empty
char buffer for input `"0"`.
@som-snytt
Copy link
Contributor Author

I have to assume that push -f is somehow evil. And it's totally awesome.

I think synthax is the poisonous white powder that parser terrorists send in the mail. I don't think it's been weaponized for email yet. @adriaanm

@adriaanm adriaanm changed the title SI-9015 Test for rejected literals and clean up parser SI-9015 Reject 0x and minor parser cleanup Dec 5, 2014
@adriaanm
Copy link
Contributor

adriaanm commented Dec 5, 2014

Thanx!

@adriaanm adriaanm self-assigned this Dec 5, 2014
adriaanm added a commit that referenced this pull request Dec 9, 2014
SI-9015 Reject 0x and minor parser cleanup
@adriaanm adriaanm merged commit 159a009 into scala:2.11.x Dec 9, 2014
@som-snytt som-snytt deleted the issue/multizero branch February 9, 2015 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants