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

Fully featured support for float #3

Closed
ndmitchell opened this issue Mar 1, 2021 · 8 comments
Closed

Fully featured support for float #3

ndmitchell opened this issue Mar 1, 2021 · 8 comments
Labels
good first issue Good for newcomers spec Violations from the Starlark spec

Comments

@ndmitchell
Copy link
Contributor

From the spec:

The Starlark floating-point data type represents an IEEE 754 double-precision floating-point number. Its type is "float".

We don't yet support floating point numbers.
@ndmitchell ndmitchell added good first issue Good for newcomers spec Violations from the Starlark spec labels Mar 1, 2021
@nataliejameson
Copy link

nataliejameson commented Mar 1, 2021 •
edited
Loading

Is this supported in the java implementation in Bazel? Fine to add, but something to keep in mind as a potential compatibility issue.

@ndmitchell ndmitchell mentioned this issue Mar 1, 2021
Closed
@pierd
Copy link
Contributor

pierd commented Sep 10, 2021

Is anyone looking into this one? I was thinking about taking a stab at it.

@ndmitchell I'm assuming we are looking into it being a proper double precision floating-point number (aka f64 in rust). Meaning that, unlike the current integer implementation, it will have to be allocated on the heap. Is that correct?

@ndmitchell
Copy link
Contributor Author

I don't think anyone is looking at it right now. Patch most welcome!

Yep, plan would be f64 and allocated on the heap. While int is ubiquitous in Starlark, and thus worth optimising a lot, my guess is most things in Starlark won't use float, so it's not a big deal, and we have optimised our heap a lot.

@pierd pierd mentioned this issue Sep 16, 2021
Closed
facebook-github-bot pushed a commit to facebookincubator/gazebo that referenced this issue Sep 16, 2021
@pierd @facebook-github-bot
Implement AnyLifetime for floats (#3)
5a566f8 
Summary:
... to simplify implementation of [floats in starlark-rust](facebook/starlark-rust#3).

Pull Request resolved: #3

Reviewed By: blackm00n

Differential Revision: D30990124

Pulled By: ndmitchell

fbshipit-source-id: c365e39f44eb86ce173d2b04247a98d848f689e3
facebook-github-bot pushed a commit to facebookincubator/antlir that referenced this issue Sep 16, 2021
@pierd @facebook-github-bot
Implement AnyLifetime for floats (#3)
1b73daf 
Summary:
... to simplify implementation of [floats in starlark-rust](facebook/starlark-rust#3).

Pull Request resolved: facebookincubator/gazebo#3

Reviewed By: blackm00n

Differential Revision: D30990124

Pulled By: ndmitchell

fbshipit-source-id: c365e39f44eb86ce173d2b04247a98d848f689e3
@pierd
Copy link
Contributor

pierd commented Sep 26, 2021

A quick update on how I'm going with this.

The main implementation is done. Lexer, parser, float function, / operator, float and int arithmetic operations are all functional.

Judging by float.star from starlark-go the missing bits are:

  • str for floats - my current implementation uses Rust's f64::to_string which is not conformant to starlark spec
  • float specific format strings (%e, %f, %g)
  • using float as a dictionary key
  • explicit failure when indexing strings/ranges/lists with float

I'm going to continue working on the above.

@ndmitchell
Copy link
Contributor Author

Thanks for the info - sounds great! Note that 100% conformance with float.star is not necessary, or sometimes even a good idea - we tend to aim for https://github.com/bazelbuild/starlark/blob/master/spec.md#contents, which is mostly but not 100% equivalent.

  • Formatting like Go seems fine, but the spec doesn't seem to be prescriptive, so up to you.
  • https://github.com/bazelbuild/starlark/blob/master/spec.md#string-interpolation has the spec on string formatting. I'd be happy to have that as an update afterwards, or a patch initially, whatever suits you.
  • For dictionaries, I imagine if you give it a hash instance it will just work.
  • Do floats support to_int or not? I would guess not? Which would give you the failures for free?

@pierd
Copy link
Contributor

pierd commented Sep 30, 2021

I've removed to_int from float - you are right, it gives us the indexing failures for free. It does break %d string interpolation for floats though. It also means that float needs special handling in int() function (not too complex - added).

I've extracted get_hash implementation for ints and floats to a separate helper to make sure they always match. Now using floats as dictionary keys work as expected (together with duplicate keys detection).

With the above the missing bits are:

  • string interpolations for float, that is the float specific ones (%e, %f, %g) and truncating float to int for integer ones (%d, %o, %x)
  • str for float which according to the spec should work just like %g string interpolation

I think implementing these as a separate update is a good idea.

I'll clean up the code a bit and prepare a pull request so we can discuss the actual implementation.

@pierd pierd mentioned this issue Sep 30, 2021
Closed
facebook-github-bot pushed a commit that referenced this issue Oct 5, 2021
@pierd @facebook-github-bot
Add support for floats (#34)
da8000d 
Summary:
Addresses: #3
Spec: https://github.com/bazelbuild/starlark/blob/689f54426951638ef5b7c41a14d8fc48e65c5f77/spec.md#floating-point-numbers

Changes:
- adds lexer support for float literal
- adds `/` and `/=` operators for division
- adds floats and division operators support to parser
- adds `float` function
- updates int arithmetic and comparisons to account for the other operand being float
- implements `SimpleValue` for float (backed by `f64`)
- adds `assert_lt` implementation (required for some conformance tests)
- adds `float.star` from starlark-go (with some formatting for easier nonconformant line removal)

Known issues:
- `str` for floats doesn't conform to the spec
- string interpolation is not implemented for floats

Pull Request resolved: #34

Reviewed By: krallin

Differential Revision: D31345070

Pulled By: ndmitchell

fbshipit-source-id: 510b0533375265187db26534c1d4683a0f2cf49a
@ndmitchell ndmitchell changed the title No support for float Fully featured support for float Oct 5, 2021
@pierd pierd mentioned this issue Oct 13, 2021
Closed
facebook-github-bot pushed a commit that referenced this issue Oct 14, 2021
@pierd @facebook-github-bot
Implement string interpolation for floats (#39)
8c89e4d 
Summary:
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

Pull Request resolved: #39

Reviewed By: krallin

Differential Revision: D31645493

Pulled By: ndmitchell

fbshipit-source-id: d47272d4bf301085544acfcfbaf17f1c2577f692
@pierd
Copy link
Contributor

pierd commented Oct 21, 2021

@ndmitchell is there anything left here or should we close this issue?

@ndmitchell
Copy link
Contributor Author

Closed - thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good for newcomers spec Violations from the Starlark spec
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@pierd @ndmitchell @nataliejameson