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

jv_parse: let decNumberFromString/strtod parse complex nans as a NaN #2985

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

emanuele6
Copy link
Member

Before this patch (when using decNumber), "Nan123" was parsed as a NaN,
only if the first n was uppercase.

@emanuele6
Copy link
Member Author

emanuele6 commented Dec 13, 2023

Initially, I also added this test:

# also "nan with payload" #2985
fromjson | isnan
"nan1234"
true

But then I removed it because I realised "nan with payload" is parsed differently on the non-decnumber build (nan(0x4D2)), so the test would be decnumber specific.

Now I realise that many other tests in jq.test will only work in decnumber builds; we should probably move them to another file like we do for onig.test.

@emanuele6 emanuele6 added this to the 1.7.1 release milestone Dec 13, 2023
@emanuele6 emanuele6 changed the title jv_parse: let decNumberFromString/strtod complex nans as a NaN jv_parse: let decNumberFromString/strtod parse complex nans as a NaN Dec 13, 2023
@wader
Copy link
Member

wader commented Dec 13, 2023

Now I realise that many other tests in jq.test will only work in decnumber builds; we should probably move them to another file like we do for onig.test.

Yes probably a good idea to split them out. We do have https://github.com/jqlang/jq/blob/master/.github/workflows/decnum.yml but it seems to not run the standard tests... maybe it should?

@emanuele6 emanuele6 requested a review from wader December 13, 2023 16:35
src/jv_parse.c Show resolved Hide resolved
@wader
Copy link
Member

wader commented Dec 13, 2023

Should we add some kind of tests also?

@emanuele6
Copy link
Member Author

It used to have tests, but I removed it because it would be decNumber specific; I guess I could revert it for now since other tests in shtest are decNumber specific

@wader
Copy link
Member

wader commented Dec 13, 2023

Yeap i think that would be good. After release we can maybe split out things and also make the decnum action (that builds without decnum support) run tests

Before this patch (when using decNumber), "Nan123" was parsed as a NaN,
only if the first n was uppercase.
@emanuele6 emanuele6 merged commit c5fd64b into jqlang:master Dec 13, 2023
28 checks passed
@emanuele6 emanuele6 deleted the paynanny branch December 13, 2023 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants