Repository navigation
Fix double rounding of number literals with more than 17 digits - #3695
Open
NgoQuocViet2001 wants to merge 1 commit into
Open
NgoQuocViet2001 wants to merge 1 commit into
NgoQuocViet2001 wants to merge 1 commit into
Conversation
Converting a decNumber literal to a double first rounded it to 17 significant digits and then passed that string to strtod. When the 17-digit result lands on or past the midpoint between two doubles, the second rounding picks the farther one, so 100000000000000041 * 1 gave 100000000000000030 instead of 100000000000000050. The --disable-decnum build already returns the nearest double. Pass every digit of the literal to strtod, which rounds correctly, and drop the now unused 17-digit decNumber type.
This branch has not been deployed
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.
Converting a number literal to a double first rounds it to 17 significant digits with
decNumberReduce(), then passes that string tojvp_strtod(). When the 17-digit value lands on or past the midpoint between two doubles, the second rounding goes the wrong way:The nearest double is 100000000000000048. Rounding to 17 digits first gives 100000000000000040, an exact midpoint, and ties-to-even then picks 100000000000000032. A
--disable-decnumbuild already prints 100000000000000050. The 17-digit step came from #2949, where it was meant to avoid allocating for digits that cannot change the result, but near a midpoint they can.This passes every digit of the literal to
jvp_strtod(), which rounds correctly. Literals with up to 17 digits still use the stack buffer; longer ones use a temporary heap buffer ofdigits + 15bytes, the same sizejvp_literal_number_literal()allocates. The unuseddecNumberDoublePrecisiontype andDEC_NUMBER_DOUBLE_EXTRA_UNITSare removed.The new case in
tests/jq.testfails before the change and passes with and without decNumber.Ran (Ubuntu 24.04, gcc 13.3):
make check: 9/9 suites pass,tests/jq.test599/599. Without the fix it is 598/599, with all four new values wrong.make checkwith--disable-decnum: 9/9 pass.make checkwith--enable-asan --enable-ubsan: 9/9 pass, no new sanitizer reports.Checked:
float(): 680 wrong before, 0 after.-0,1E+1000,-1E+1000,1E-1000, subnormals, values nearDBL_MAXand1e999999999give the same results as before.