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: some bugs in detach and replace #456

Merged
merged 3 commits into from
May 6, 2020
Merged

Conversation

miaoerduo
Copy link
Contributor

@miaoerduo miaoerduo commented Apr 3, 2020

  1. set list head's prev when parse_object and parse_array
  2. detach the first item when array size is 1
  3. detach the last item
  4. replace the first item when array size is 1
  5. replace the last item

@miaoerduo miaoerduo changed the title feat: set list head's prev in parse_array and parse_object fix: some bugs in detach and replace Apr 3, 2020
@@ -1509,6 +1509,10 @@ static cJSON_bool parse_array(cJSON * const item, parse_buffer * const input_buf
success:
input_buffer->depth--;

if (head != NULL) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@miaoerduo thanks your suggestion! but this change make the testcase fail, I have to make some changes for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the 0th is unnecessary? How about the others? I have test these in my project. :P

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, it's needed. compiling it with option cmake ENABLE_VALGRIND=On .. when make check, memory check is very important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I have commited with company's email.
So I have to changed the email in commits and force pushed again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May my modification be merged into this project?
Is there any other thing I could do?
I haven't contributed to a project with so many stars!

Copy link
Collaborator

Choose a reason for hiding this comment

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

This version is released no more than 24 hours since you make this contribution, you have done a good job :), before I merge it, I need some time to make sure it won't introduce any problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow! Thank YOU very much!

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