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.

Sign up for GitHub

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

Jump to bottom

Parse nan in JSON as NaN instead of triggering a parse error #2712

Merged
merged 2 commits into from Jul 13, 2023
Merged

Parse nan in JSON as NaN instead of triggering a parse error #2712

merged 2 commits into from Jul 13, 2023

Conversation

emanuele6
Copy link
Member

Fixes #2021

@emanuele6 emanuele6 marked this pull request as draft July 13, 2023 17:21
@emanuele6
Copy link
Member Author

I wrote a test that uses fromjson/1, and it failed because it triggers a memory leak, I am investigating

@emanuele6
Fix memory leak for has(nan)
2233539 
jv_array_get() used to be responsible of freeing the input array, but
since b5c4c3d, it is no longer called
if the key is nan. We need to free it manually to avoid leaking the
array.
@emanuele6
Parse nan in JSON as NaN instead of triggering a parse error
b4bf2ce 
Fixes jqlang#2021
@emanuele6
Copy link
Member Author

emanuele6 commented Jul 13, 2023 •
edited

Hmm, actually this was unrelated to my patch and the use of fromjson in the test; b5c4c3d introduced a memory leak.

I am not sure why the CI didn't catch it. Does the CI run tests without valgrind?

@emanuele6 emanuele6 marked this pull request as ready for review July 13, 2023 18:41
@emanuele6
Copy link
Member Author

Yes, CI configures with --disable-valgrind; it should probably not be disabled, no?

itchyny
itchyny approved these changes Jul 13, 2023
View reviewed changes
Copy link
Contributor

@itchyny itchyny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@itchyny itchyny merged commit 4b1ac7c into jqlang:master Jul 13, 2023
24 checks passed
@emanuele6 emanuele6 deleted the naninjson branch July 13, 2023 22:14
@nicowilliams
Copy link
Contributor

nicowilliams commented Jul 13, 2023 •
edited

Yes, CI configures with --disable-valgrind; it should probably not be disabled, no?

I would make one workflow not do that. Not all of them, because there's no need to waste that much build capacity.

@emanuele6
Copy link
Member Author

@nicowilliams Makes sense; also only the amd64 CI runs tests anyway.

@emanuele6
Copy link
Member Author

Hmm, afaict --disable-valgrind only matters if you run make check. So the fix is to remove --disable-valgrind for all workflows anyway. There is no need to conditionally disable it with something like ${{ matrix.arch != 'amd64' && "--disable-valgrind" }} \.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itchyny itchyny itchyny approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inconsistent behavior with NaN
3 participants
@emanuele6 @nicowilliams @itchyny