Apparently our naive implementation of float formatting, which simply
used {:.5}, and trimmed trailing "0" strings not sufficient.
It wrongly trimmed numbers with zeroes but no decimal point, like
`10000` got trimmed to `1`.
Nix uses `std::to_string` on the double, which according to
https://en.cppreference.com/w/cpp/string/basic_string/to_string
is equivalent to `std::sprintf(buf, "%f", value)`.
https://en.cppreference.com/w/cpp/io/c/fprintf mentions this is treated
like this:
> Precision specifies the exact number of digits to appear after
> the decimal point character. The default precision is 6. In the
> alternative implementation decimal point character is written even if
> no digits follow it. For infinity and not-a-number conversion style
> see notes.
This doesn't seem to be the case though, and Nix uses scientific
notation in some cases.
There's a whole bunch of strategies to determine which is a more compact
notation, and which notation should be used for a given number.
https://github.com/rust-lang/rust/issues/24556 provides some pointers
into various rabbit holes for those interested.
This gist seems to be that currently a different formatting is not
exposed in rust directly, at least not for public consumption.
There is the
[lexical-core](https://github.com/Alexhuszagh/rust-lexical) crate
though, which provides a way to format floats with various strategies
and formats.
Change our implementation of `TotalDisplay` for the `Value::Float` case
to use that. We still need to do some post-processing, because Nix
always adds the sign in scientific notation (and there's no way to
configure lexical-core to do that), and lexical-core in some cases keeps
the trailing zeros.
Even with all that in place, there as a difference in `eval-okay-
fromjson.nix` (from tvix-tests), which I couldn't get to work. I updated
the fixture to a less problematic number.
With this, the testsuite passes again, and does for the upcoming CL
introducing builtins.fromTOML, and enabling the nix testsuite bits for
it, too.
Change-Id: Ie6fba5619e1d9fd7ce669a51594658b029057acc
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7922
Tested-by: BuildkiteCI
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: tazjin <tazjin@tvl.su>
Using `serde_json` for parsing JSON here, plus an `impl FromJSON for
Value`. The latter is primarily to stay "dependency light" for now -
likely going with an actual serde `Deserialize` impl in the future is
going to be way better as it allows saving significantly on intermediary
allocations.
Change-Id: I152a0448ff7c87cf7ebaac927c38912b99de1c18
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6920
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
