Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[FS-1142] Extended numeric literals #770
base: main
Are you sure you want to change the base?
[FS-1142] Extended numeric literals #770
Changes from 2 commits
c99355f
42d3f87
ad3fd6e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel strongly that we should put the floating point part (possibly with decimals) in a separate RFC. It is significantly different to extend the domain, vs, just updating a bit of syntax (i.e.,
0x
and0b
are just syntax).Can you also expand if the previous point applies to custom literals as well, and if, in that case, the trailing
I
orG
can have an underscore before it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. there can be some underscores before
I
orG
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
floats: please put that in a separate RFC (and later also, PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is not a breaking change, because it was previously not possible to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although the bignum (
123I
) already known how to deal with the number prefix (0x123I
is now ok without changing the library), other custom numeric literals may not know that. This might break their function, so I think it is a breaking change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is, this is not a break change for existing source code, but it is for library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put this in a separate RFC, it is rather separate from the other things here (it shares the same subject, but smaller RFCs with less scope have a larger change of "making it", same is true for implementation PRs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the breaking change?
Despite my comment above, I think you are right: if we pass the strings with the prefix, this may lead to existing modules not understanding this syntax. This can be explained in the section below (i.e.: if existing modules encounter the new syntax, they may fail).
However, existing code will never fail, so in that sense, it is not a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's possible, but I think it is a bad idea, precisely because currently, only integers are allowed anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a much better suggestion. In fact, you don't even have to do that. Conversion is rather trivial and can be done on-the-fly.
This would certainly be my preference and would avoid any burden on people writing custom numerals modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try to avoid this, if possible, see my comments and trains of thought above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's revisit this after we have a decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is probably not going to be a problem, as the syntax highlighter uses the parser results. If the parser says something is a numeric literal, it will be colored as such.
But certainly something to test before merging this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion is: no