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

Drop uppercase F32 and F64 float number suffixes #8782

Merged
merged 2 commits into from
Feb 12, 2020

Conversation

rhysd
Copy link
Contributor

@rhysd rhysd commented Feb 11, 2020

With Crystal 0.32.1,

# OK
0f32

# OK
1f32

# ERROR
0F32

# OK
1F32

This PR fixes 0F32 not to cause an error. EDIT: We decided to drop uppercase suffixes. Please see following discussion.

@rhysd rhysd force-pushed the 0F32 branch 2 times, most recently from f51371a to c904103 Compare February 11, 2020 04:46
@rhysd
Copy link
Contributor Author

rhysd commented Feb 11, 2020

Hmm, I'm not able to locate which test case fails on CircleCI.

https://circleci.com/gh/crystal-lang/crystal/38220?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Error while trying to determine if a stack overflow has occurred. Probable memory corrpution

Invalid memory access (signal 11) at address 0x40

[0x5621f5b60026] *CallStack::print_backtrace:Int32 +118

[0x5621f55889cc] __crystal_sigfault_handler +348

[0x5621f72a9fe4] sigfault_handler +40

[0x7f349bd970d4] ???

@straight-shoota
Copy link
Member

As far as I know, this is intended. Type suffixes use only lower case characters. That also affects all similar identifiers. What makes you think this needs to support upper case?

test_alpine failing is not related to this PR, you can ignore it.

@asterite
Copy link
Member

As far as I know, this is intended. Type suffixes use only lower case characters.

It seems 2F32 and 4F64 parse well, but using capital I for integer does not. So I think we should actually drop the capital F in those cases.

@rhysd
Copy link
Contributor Author

rhysd commented Feb 11, 2020

@straight-shoota @asterite

Actually 4.0F32 is accepted. I'm not understanding the intention, but lexer implementation explicitly accepts uppercase.

So I thought 'F' should be accepted in the case of 0F32.

I agree that uppercase suffix looks weird. There is no test for F32 and F64 suffixes. So I feel it's better to drop uppercase suffix.

But it is a breaking change. Should we do it? I'm sorry that I'm not understanding how Crystal dev team currently makes a decision around breaking changes. If we should do, I'll update this PR to drop the uppercase F32 and F64 suffixes. Modification would be easy.

@asterite
Copy link
Member

Yes, I think we should drop F, even if it's a breaking change. Probably not many use it or know about it, and fixing it is easy.

@rhysd
Copy link
Contributor Author

rhysd commented Feb 11, 2020

OK, I'll update this branch tonight.

@rhysd rhysd changed the title Fix 0F32 causes an error Drop uppercase F32 and F64 float number suffixes Feb 11, 2020
@rhysd
Copy link
Contributor Author

rhysd commented Feb 11, 2020

@straight-shoota @asterite

I modified this PR for dropping the uppercase suffixes. And I also added small tests for invalid float number suffixes. Would you review this?

@straight-shoota
Copy link
Member

I'm almost sure upper case characters are unlikely to be in use somewhere. But we should at least consider a deprecation path, either updating the formatter first or providing a deprecation message when an upper case character is encountered.
IMO it's not necessary in this case because it's completely undocumented and probably not used.

@bcardiff
Copy link
Member

If some shard is using F, it can be changed to f and publish a patch release if needed. That will also work in previous crystal versions.

I'm fine with merging this right away.

@rhysd
Copy link
Contributor Author

rhysd commented Feb 12, 2020

CI fails due to #8782 (comment) but it seems not related to changes in this PR since in other PR it also fails with the same error. It's weird that #8781 does not cause the error, tho.

@asterite asterite added this to the 0.33.0 milestone Feb 12, 2020
@asterite asterite merged commit 39f3605 into crystal-lang:master Feb 12, 2020
rhysd added a commit to vim-crystal/vim-crystal that referenced this pull request Feb 13, 2020
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.

4 participants