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

Fix precision issues with filsize and duration literals #12872

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

IanManske
Copy link
Member

@IanManske IanManske commented May 15, 2024

Description

As the title says, this PR fixes issues with fractional values in filesize and duration literal values. E.g., 0.5wk is now reported as 3day 12hr instead of 3day.

Fixes #12858, fixes #10612, and fixes #9892.

Implementation wise, this splits the Unit type into FilesizeUnit and DurationType. These types have a few helpful methods and are now used a few other places besides the parser. Additionally, this PR adds the Expr::Filesize and Expr::Duration cases to replace the existing Expr::ValueWithUnit.

User-Facing Changes

Breaking changes:

  • Filesizes and durations now follow the same rules as float parsing. E.g., -.0GiB used to not parse as a filesize even though -.0 is parsed as a valid float.
  • Filesize and duration literals that are out of the range of an i64 number of bytes or nanos now trigger a parser error.
  • Fixed the precision issues with filesize and duration literals. Although this is a bug fix, it is still technically breaking since it changes the values of existing code.
  • format filesize now errors on an invalid filsize unit instead of silently continuing.

Current (Known) Limitations

  • The whole number part of filesize or duration cannot be outside the range of an i64 even if the exponent is negative. E.g., 9223372036854775808e-1B will give a parser error.
  • The value of 10^abs(exponent) must not be larger then i64::MAX, otherwise an error is reported. The means that the only valid exponents are in the range [-18, +18].

@IanManske IanManske added pr:breaking-change This PR implies a change affecting users and has to be noted in the release notes pr:bugfix This PR fixes some bug pr:commands This PR changes our commands in some way pr:errors This PR improves our error messages pr:language This PR makes some language changes labels May 15, 2024
@sholderbach
Copy link
Member

Dang I should have gotten #12475 landed before that. Have to look into the details of how you are working the parsing now but the type conversions and base changes (base 10/base 2 with modf) from #6640 seem fishy to me in regards to numerical stability

@IanManske
Copy link
Member Author

Dang I should have gotten #12475 landed before that

This is still draft, feel free to make any necessary changes to that PR to land it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:breaking-change This PR implies a change affecting users and has to be noted in the release notes pr:bugfix This PR fixes some bug pr:commands This PR changes our commands in some way pr:errors This PR improves our error messages pr:language This PR makes some language changes
Projects
None yet
2 participants