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.

Sign up for GitHub

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

Jump to bottom

Implement string interpolation for floats #39

Closed
wants to merge 2 commits into from
Closed

Implement string interpolation for floats #39

wants to merge 2 commits into from

Conversation

pierd
Copy link
Contributor

@pierd pierd commented Oct 13, 2021

Addresses: #3
Spec: https://github.com/bazelbuild/starlark/blob/689f54426951638ef5b7c41a14d8fc48e65c5f77/spec.md#string-interpolation

Changes:

  • adds float::write_{decimal,scientific,compact} functions for various kinds of string interpolation for floats
  • makes Display for StarlarkFloat use write_compact (as per spec)
  • makes string interpolation use write_* functions

pierd
pierd commented Oct 13, 2021
View reviewed changes
starlark/testcases/eval/go/float.star
assert.eq(str(sorted([inf, neginf, nan, 1e300, -1e300, 1.0, -1.0, 1, -1, 1e-300, -1e-300, 0, 0.0, negzero, 1e-300, -1e-300])), "[-inf, -1e+300, -1.0, -1, -1e-300, -1e-300, 0, 0.0, -0.0, 1e-300, 1e-300, 1.0, 1, 1e+300, +inf, nan]")
assert.eq(
str(sorted([inf, neginf, nan, 1e300, -1e300, 1.0, -1.0, 1, -1, 1e-300, -1e-300, 0, 0.0, negzero, 1e-300, -1e-300])),
"[-inf, -1e+300, -1.0, -1, -1e-300, -1e-300, 0, 0.0, -0.0, 1e-300, 1e-300, 1.0, 1, 1e+300, +inf, nan]")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Restored the original since we no longer ignore this test.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 13, 2021
ndmitchell
ndmitchell approved these changes Oct 13, 2021
View reviewed changes
Copy link
Contributor

@ndmitchell ndmitchell left a comment

Choose a reason for hiding this comment

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

Awesome work! Very impressive. As before, I'll let you address/comment on the issues below, then once you've updated, mirror it in to FB side and merge.

starlark/src/values/types/float.rs Outdated Show resolved Hide resolved
starlark/src/values/types/float.rs Outdated Show resolved Hide resolved
@pierd
Copy link
Contributor Author

pierd commented Oct 13, 2021

@ndmitchell has anything changed in ci checks? I've noticed that clippy found a lot of problems that are not related to changes in this PR.

@ndmitchell
Copy link
Contributor

The clippy changes are partly due to nightly clippy detecting more things, and partly due to code changes. I think we're down to one in the latest HEAD, as I was working on it yesterday. Don't worry too much - we have a set of lints we apply internally too, so that will catch the ones we care about.

@facebook-github-bot
Copy link
Contributor

@ndmitchell has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@ndmitchell merged this pull request in 8c89e4d.

@ndmitchell
Copy link
Contributor

Thanks for the code! All landed and most appreciated!

In internal code review we debated powf vs powi for a while, before realising powi in Rust is totally broken - rust-lang/rust#71355 (comment). I also flipped from a Vec to an [u8; 6] so that we save a malloc in printing, at someones request. There was an additional test case too. But very minor tweaks.

@pierd pierd deleted the float-str branch October 14, 2021 22:07
@pierd
Copy link
Contributor Author

pierd commented Oct 14, 2021

Yeah, I learned about powi the hard way too (that's what I used initially). Great idea to use [u8; 6] instead of a Vec since we know the max size during compilation anyway 🎉

Happy to help as always

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndmitchell ndmitchell ndmitchell approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pierd @ndmitchell @facebook-github-bot