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

Fix error pointer behaviour of cJSON_ParseWithOpts() #200

Merged

Conversation

rmallins
Copy link

@rmallins rmallins commented Sep 7, 2017

What is
cJSON_ParseWithOpts() doesn't set the error pointer on parse error. This differs from cJSON_Parse() and also from original behaviour.
What should be
cJSON_ParseWithOpts() should set the error pointer on parse error.

Detail
I started using cJSON at work around a year and a half ago, and wrote unit tests around that version of the code. When I updated my copy of the source with the latest code from this repo last week I noticed that my tests failed due to the change in error handling behaviour for cJSON_ParseWithOpts():

original behaviour on error

  • parse_end pointer == NULL
  • error pointer == last parsed byte + 1

current behaviour on error

  • parse_end pointer == last parsed byte + 1
  • error pointer == NULL

proposed behaviour on error

  • parse_end pointer == last parsed byte + 1
  • error pointer == last parsed byte + 1

It makes good sense that the parse_end pointer is now updated. However it looks like a bug to fail to update the error pointer in the error case. It violates the principle of least surprise, and is also an interface change for no good reason that I can see.

My change updates the unit tests to check for the new behaviour, and updates the code to match.

I developed against master before discovering a minute ago that you need changes to go on the "develop" branch, so I submitted the merge request to develop. As will be obvious it's only the last 3 commits that are mine. My apologies for the clutter!

regards,
Robin Mallinson

@FSMaxB FSMaxB added the bug label Sep 7, 2017
@FSMaxB
Copy link
Collaborator

FSMaxB commented Sep 7, 2017

You are right, it is inconsistent not to set the global error pointer if a return_parse_end was given.

I regard this as a bug fix, not an intrusive change, so please redo the pull request on top of master (sorry for the confusion).

Also please ensure that every single commit passes the unit tests --> combine the RED and GREEN commits into one commit.

Thanks for your fixes.

If you have more unit tests lying around, please let me know. More pull requests welcome.

@rmallins rmallins force-pushed the cJSON_ParseWithOpts_fix_ErrorPtr_behaviour branch from 6630319 to 218b245 Compare September 7, 2017 21:41
@rmallins rmallins changed the base branch from develop to master September 7, 2017 22:20
@rmallins
Copy link
Author

rmallins commented Sep 8, 2017

OK, have merged the 2nd and 3rd changes into one commit and reinstated the change over master. I doubt I can deliver the unit tests I've written at work but if I get some spare time I'll see what I can add on my own time.

rmallins added 2 commits September 8, 2017 01:20
@rmallins rmallins force-pushed the cJSON_ParseWithOpts_fix_ErrorPtr_behaviour branch from 218b245 to 629c354 Compare September 8, 2017 00:24
@FSMaxB
Copy link
Collaborator

FSMaxB commented Sep 8, 2017

Oh, I forgot that the appveyor.yml is only on the develop branch. So ignore the failed AppVeyor build.

@FSMaxB FSMaxB merged commit e4980b6 into DaveGamble:master Sep 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants