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

Allow ties in floating literals #866

Merged
merged 1 commit into from
Oct 2, 2021
Merged

Allow ties in floating literals #866

merged 1 commit into from
Oct 2, 2021

Conversation

zygoloid
Copy link
Contributor

@zygoloid zygoloid commented Oct 1, 2021

Allow ties in floating-point literals, and round them to even, per the default IEEE rounding mode rules.

The rule from #143 that we should reject such cases turns out to reject reasonable code, such as:

var v: f32 = 9.0e9;
var w: f64 = 5.0e22;

@zygoloid zygoloid added the proposal A proposal label Oct 1, 2021
@zygoloid zygoloid requested a review from a team October 1, 2021 19:23
@google-cla google-cla bot added the cla: yes PR meets CLA requirements according to bot. label Oct 1, 2021
@zygoloid zygoloid marked this pull request as ready for review October 1, 2021 20:13
@zygoloid zygoloid requested a review from a team as a code owner October 1, 2021 20:13
@github-actions github-actions bot added the proposal rfc Proposal with request-for-comment sent out label Oct 1, 2021
@chandlerc
Copy link
Contributor

Needs a summary, but otherwise seems good and I think uncontroversial.

Comment on lines +57 to +67
... because the literal arithmetic would be performed exactly, resulting in the
same tie. A workaround such as

```
var v1: f32 = 5.0e22 + 1.0;
var v2: f32 = 5.0e22 - 1.0;
```

... to request rounding upwards and downwards, respectively, would work.
However, these seem cumbersome and burden the Carbon developer with
floating-point minutiae about which they very likely do not care.
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I actually think math between floating point literals is even more important than just removing another way to write this... It seems likely to make it much more likely to encounter. It may not be reasonable to somehow change arithmetic to not produce evenly between values.

Anyways, not disagreeing, just suggesting its even better motivated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree that working around such things will be awkward. On the other hand, we'd still need to refute the statistical argument from #143, that such exact ties are unlikely to come up by chance if your computation doesn't put heavy skew into the distribution of resultant mantissas. I don't think it's going to be all that hard to contrive a computation where that'd happen, but I think such easy-to-find examples are likely to amount to computing one of the simple base-10 values that hit this problem or a variant of one of those.

Copy link
Contributor

Choose a reason for hiding this comment

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

(discussed, and fine to move forward as is for now)

Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

LGTM, this seems uncontroversial, so ship it.

@zygoloid zygoloid merged commit 27f9ca6 into carbon-language:trunk Oct 2, 2021
@zygoloid zygoloid deleted the proposal-allow-ties-in-floati branch October 2, 2021 18:01
@github-actions github-actions bot added proposal accepted Decision made, proposal accepted and removed proposal rfc Proposal with request-for-comment sent out labels Oct 2, 2021
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes PR meets CLA requirements according to bot. proposal accepted Decision made, proposal accepted proposal A proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants