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

Combine json_alloc_token and json_fill_token #211

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alexhenrie
Copy link

JSMN has been initializing tokens with default values, then initializing them again with the real values. The first step can be skipped to make JSMN smaller and faster.

@pt300
Copy link
Collaborator

pt300 commented Aug 30, 2021

Interesting proposal. Have you checked that JSMN never really depends on those default values in cases such as missing JSON?

@alexhenrie
Copy link
Author

Two of the three calls to json_fill_token are immediately after json_alloc_token. The only place where json_fill_token is called later, and therefore behavior could change, is when JSMN_STRICT is defined and an object or array is used as a key. In that case, token->type was previously uninitialized and token->start was initialized to -1 before returning JSMN_ERROR_INVAL. With this patch, token->type and token->start are set before erroring out.

JSMN itself doesn't do anything with those values before returning, so really we're just talking about what the caller should see if the caller looks at the tokens despite jsmn_parse returning JSMN_ERROR_INVAL. In my opinion, initializing the last token (the bad key) with accurate values before returning an error is more useful than leaving the values uninitialized or invalid. However, if it's important, I could easily reset token->type to JSMN_UNDEFINED and token->start to -1 on the error path. Just let me know and I'll modify the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants