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

Check for trailing garbage (comma) at the end of objects and arrays, before closing bracket #156

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

tmegow
Copy link

@tmegow tmegow commented Dec 7, 2021

This replaces #80 and #149. I've included all of @DimitriPapadopoulos 's suggestions from the review of #80 and #149.

json.c Outdated Show resolved Hide resolved
json.c Outdated Show resolved Hide resolved
json.c Outdated Show resolved Hide resolved
json.c Outdated Show resolved Hide resolved
@LB--
Copy link
Member

LB-- commented Dec 9, 2021

The CIs fail because this line needs to be bumped from 10 to 12. (The number of files is hard-coded instead of autodetected on purpose, to make sure rare filesystem errors don't mess things up.)

As discussed elsewhere this library is a parser and not a linter. This change is currently unconditional, so it will waste performance for people who just want to parse data and don't care about extraneous commas. It may be better to have it as a parser option (like json_enable_comments), but that would still add an extra branch to the code path, so perhaps if this is really desired it could be hidden behind both a parser option and a compile definition, so users first have to compile with the extra code and extra branch, and then they can at runtime choose whether to run the check or not. And depending on how easy it would be, we could have a third option via compile definition so that the branch is eliminated that checks for the parser option, and the linter just always runs.

Perhaps something like:

Value of JSON_LINTING Parser option present If branch for option Linting code present
0 No No No
1 Yes Yes Yes (runs conditionally)
2 No No Yes (runs always)

However, I am wondering if this even makes sense to support in the first place. I think dedicated linters could be faster anyway as they wouldn't need to parse out the data at all, just verify that it is syntactically valid. I'm not sure how much sense it makes to try and do two things at once and compete with both parsers and linters, since it is rare to need both at the same time. With some effort we certainly could, but is it worth it?

@tmegow
Copy link
Author

tmegow commented Dec 9, 2021

Thank you for your review! Regarding whether the change makes sense because dedicated linters can be used, I think I will trust your judgement.

When I looked for a C json parsing library and found json-parser, I did begin using it with the expectation that the parser would fail when consuming invalid ECMA-404 JSON. I went to file an issue and found the existing #73 . I am only using json-parser for a silly personal project so I decided it would be a fun challenge to try to make a PR addressing the issue. When approaching the problem, I initially tried to adjust the parsing logic so these trailing commas could be detected with minimal performance impact. I unsuccessfully tried using pointer incrementing to look ahead, but I couldn't find a way to make it more efficient than moving the pointer backwards from closing brackets and braces. I am very curious, for my own education, if you have a better way to implement this goal?

I will add the compile definition and the parser option to this branch like you describe. 👍

@LB--
Copy link
Member

LB-- commented Dec 9, 2021

I am very curious, for my own education, if you have a better way to implement this goal?

I definitely feel like there must be a simpler way, as I thought the parser already tracks whether it has seen a comma or not. I haven't investigated it much though, I will do so later.

…nt upon a compile-time option. Enable runtime option for enabling linting when compile-time option is set. Make compile-time option accept values of unset, 1, or 2 - with 1 requiring the runtime option to be included for linting to run and with 2 loading and running the linting code without requiring the runtime option
@tmegow
Copy link
Author

tmegow commented Dec 19, 2021

Hey @LB-- Happy Holidays! I pushed an attempt to implement the changes you requested.

I put the loading of the conditional linting code branches behind a compile-time option, JSON_LINTING.

I also made the runtime option, json_enable_linting, which is set using settings.settings just like json_enable_comments is.

Setting JSON_LINTING to 1 or 2 loads the conditional code branches.

JSON_LINTING set to 1 additionally requires the enabling of the json_enable_linting runtime option to perform linting.

JSON_LINTING set to 2 performs linting without requiring the runtime option.

Please review and test out the changes, is this accurate to your directions?

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.

None yet

3 participants