-
Notifications
You must be signed in to change notification settings - Fork 3.1k
SI-9015 Reject 0x and minor parser cleanup #4182
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
Conversation
bb8f61a
to
4158ddd
Compare
I see that |
@lrytz Showing that it's OK to fix bugs: 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. |
I'd vote to just go ahead fix this one without a deprecation. |
LGTM, really a great cleanup! I'm more than happy be outvoted and skip deprecation of |
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 |
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 |
(While you're at it, squash to 0x1 commit?) |
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. |
ad9bef7
to
7238bd5
Compare
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"`.
7238bd5
to
be553aa
Compare
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 |
Thanx! |
SI-9015 Reject 0x and minor parser cleanup
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.