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 for errors in streams '{\**\\**\"a":"b"}' and '{ ' with comments #46

Closed
wants to merge 1 commit into from

Conversation

fireice-uk
Copy link
Contributor

Hi,

Those are fixes for bugs mostly caused by the design issue I told you about. I added a test case that tests for multiple comment - whitespace sequences.

You should consider adding some tests that are done at the end of mmap'ed or VirtualAlloc'ed page - this way your code will segfault if it reads past the input buffer like it would on '{ '.

Fireice

state->line_no++;
state->line_offset = state->offset;
} else if ((' ' != c) && ('\r' != c) && ('\t' != c)) {
return 0;
} else if (c != ' ' && c != '\r' && c != '\t') {
Copy link
Owner

Choose a reason for hiding this comment

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

This line isn't actually changing anything (but my coding style).

for (; state->offset < state->size; state->offset++) {
const char c = state->src[state->offset];

if ('\n' == c) {
if (c == '\n') {
Copy link
Owner

Choose a reason for hiding this comment

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

This line isn't a functional change.

@fireice-uk
Copy link
Contributor Author

The functional changes are on the other lines. Those are just simple changes that I made while debugging the issue to make the function easier to understand for myself - feel free to ditch them.

The important changes are - functions return 1 if some comments or whitespace have been consumed, and the main function will keep on going until both functions say that they have nothing more to consume, or the stream ends.

Oh, and I see clang lives up to its name. Change the return type to size_t to make it happy.

@fireice-uk
Copy link
Contributor Author

One more thing - you also need a stream end check when you read the second byte in json_skip_c_style_comments - otherwise you will again read past the input buffer on '{/'

@sheredom sheredom mentioned this pull request Dec 9, 2016
@sheredom
Copy link
Owner

sheredom commented Dec 9, 2016

I've brought this functionality in as part of #49

@sheredom sheredom closed this Dec 9, 2016
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