skeeto / pdjson

C JSON parser library that doesn't suck
The Unlicense
281 stars 36 forks source link

some improvements #6

Closed aleks-f closed 7 years ago

aleks-f commented 7 years ago
aleks-f commented 7 years ago

@skeeto any chance this may be accepted/merged? We plan to embed pdjson in upcoming Poco release, so it would be nice to be aligned with upstream

skeeto commented 7 years ago

Sorry for dragging my feet. I wanted to run your changes against JSONTestSuite to make sure nothing broke. @dhobsd took charge of all of that earlier this year, and so I wasn't familiar with the process myself. I kept putting that off.

Everything still passes and it looks like you've successfully made it stricter about malformed UTF-8. Some tests with "undefined" results have switched from passing to failing. Here are the original results (light blue == passing, dark blue == failing):

original

Here it is with your PR:

updated

So I see no reason not to merge this. Thanks!

dhobsd commented 7 years ago

+1, I know this is merged; the new failures are good failures that would've been defined sensibly if JS wasn't specified around utf-16. Ideally, the rest of these would fail as well, but that's another project :)

dhobsd commented 7 years ago

Fwiw, should open another pr against the test suite for these changes since it imports libraries into the repo directly, not thru externals (if you've not done that already)

aleks-f commented 7 years ago

thanks a lot @skeeto and @dhobsd