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

Fix issue 22882 - Floating-point literals with leading zeroes incorrectly throw octal errors #13827

Closed
wants to merge 1 commit into from

Conversation

aposteriorist
Copy link

@aposteriorist aposteriorist commented Mar 16, 2022

Fixes issue #22882.

Floating-point literals with leading zeroes incorrectly throw an octal digit error, as follows:

writeln(07.0); // 7
writeln(08.0); // Error: octal digit expected, not 8
writeln(010.9); // 10.9
writeln(018.9); // Error: octal digit expected, not 8
writeln(00077777.0); // 77777
writeln(00077778.0); // Error: octal digit expected, not 8

The error is in lexer.d; errorDigit is set in number() in the initial switch statement, but when a '.' is subsequently handled later in the function, the error state is never unset.

The fix checks for base == 8 within the '.' case of the appropriate switch, and if true, unsets errorDigit and sets base to 10, as we are now working with a decimal float literal.

This is a simple fix to issue #22882, though I'm sure something cleaner could be done.
@aposteriorist aposteriorist changed the title Fix issue 22882: Floating-point leading zeroes bug Fix issue 22882 - Floating-point literals with leading zeroes incorrectly throw octal errors Mar 16, 2022
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @aposteriorist! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
22882 normal Floating-point literals with leading zeroes incorrectly throw octal errors

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#13827"

@Geod24
Copy link
Member

Geod24 commented Mar 17, 2022

This needs a test to prevent any future regression.

Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

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

Missing tests.

@dkorpel
Copy link
Contributor

dkorpel commented Mar 17, 2022

Thanks for the bugfix! As mentioned, this needs to be added to the test suite. Take the number literals from the bugzilla issue, and use them in the file test/fail_compilation/numliteral.c. You can't use writeln because dmd's tests may not import the standard library, so instead use something like this:

float x = 08.0;
static assert(00077777.0 == 77777.0);

@aposteriorist
Copy link
Author

aposteriorist commented Mar 18, 2022

Actually, I was mistaken. Floating-point literals with leading zeroes work as they should, completely corrected as of 2.098.1. I was testing from a lower version. This change is unnecessary and the pull request should be closed.

Since we're here, test/fail_compilation/numliteral.c is a test for failure to compile via ImportC, right? But the last test currently in that file,
// https://issues.dlang.org/show_bug.cgi?id=22549
int z = 078.0 + 008. + 009e1;,
successfully compiles via ImportC. Am I correct in thinking that particular test actually belongs in test/compilable/?

@dkorpel
Copy link
Contributor

dkorpel commented Mar 18, 2022

Am I correct in thinking that particular test belongs in test/compilable/ in this case?

It's not uncommon to have related positive and negative test cases in a single file in fail_compilation. There's no strict guideline on where to place test cases, it's usually judged on an individual basis based on properties like:

  • putting related tests in the same file makes it easier to see existing test coverage of a certain language feature
  • less files makes the test suite run faster and makes the test folder easier to browse
  • but a too large file might make the test too complex. It should remain simple to connect the expected output with the code of the test case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants