search

facebook / starlark-rust

A Rust implementation of the Starlark language
Apache License 2.0
699 stars 57 forks source link

Implement string interpolation for floats #39

Closed pierd closed 2 years ago

pierd commented 2 years ago

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

Changes:

pierd commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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

facebook-github-bot commented 2 years ago

@ndmitchell merged this pull request in facebookexperimental/starlark-rust@8c89e4d4fab33168a2bd391dcca58023c07bf045.

ndmitchell commented 2 years ago

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 - https://github.com/rust-lang/rust/issues/71355#issuecomment-943258728. 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 commented 2 years ago

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