Repository navigation
Avoid floating point noise when constant folding calc() and unit converstion - #20585
Open
otarampinelli wants to merge 4 commits into
Open
otarampinelli wants to merge 4 commits into
otarampinelli wants to merge 4 commits into
Conversation
Contributor
Contributor
[Medium impact] The PR appears safe to merge; no new actionable issues were found. Reviews (3) · Last reviewed commit: "Merge branch 'main' into fix/constant-fo..." · Reviewed by Greptile |
calc() and unit converstioncalc() and unit converstion
Contributor
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Constant folding on
constant-fold-declaration.tswere computing everycalc()fold and unit conversion, so decimal inputs leakead floating point noise into canonicalizationexamples:
the compiled css isn't affected, only canonicalization because it feeds the
canonicalizeCLI, the upgrade codemod and IntelliSense, so the noise could be written into users source files, it also made equivalent values compare as different:solution approach:
the operands are already decimal string in the css, so this does just the math on those strings
4.35 -> { int: 435, scale: 2 }, andand,sub,mulanddivwork on int numbers,divreturns a result only if it ends whitin 2 decimals, the same limit as the current implementation, any input that isn't a plain decimal, or any intermediate value that isn't a safe integer returnsnullcm = ×4800/127,mm = ×480/127,q = ×120/127andpt = ×4/3are exact multiplication plus one float division, round to 12 significant digits, so1cm,10mmand40q cannonicalize identically,in,pc,rem,grad,turn,msandkhzconvert exactly,radusepi, so it is computed with float and rounded to 12 significant digits, significant digits (not decimal places) keep the relative error the same at any magnitude, so a tiny converted value that acalc()multiplies back up stays correct, likecalc(0.00000000001pt * 1000000000000) → 10ptinstead of0, values that can't be written with 12 digits without an exponent aren't convertedcreateSpacingCacheis exact with a fallback to the float ration for values that aren't 2 decimal multipliers, the fallback keeps line-height matching such as28.5px / 4px = 7.125 → text-sm/[28.5px]workingNumber, so items behave like1.50 → 1.5and-0 → 0, this also stops output like1e-7,1e+200pxandInfinitystackblitz exmaple using
maincode: https://stackblitz.com/github/otarampinelli/tw-canon-repro?file=repro.mjsTest plan
•
src/utils/decimal.test.tsevery export, including each null case, unsafe integers, division by zero, maxDecimals, no exponent output, -0•
src/constant-fold-declaration.test.tsnoise free+ - * /and nested re association, the 2 decimal division limit, rem at 14 and 18, exact conversions andcm/mm/qequivalence, no exponent notation, no overflow or precision loss, and rad rounding•
src/canonicalize-candidates.test.tsend-to-end rows from the table above, plusrotate-[0.785398163397rad] → rotate-45 and rotate-[1rad]staying arbitrary.