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

use prev pointer when array do adding #430

Merged
merged 2 commits into from
Feb 18, 2020
Merged

use prev pointer when array do adding #430

merged 2 commits into from
Feb 18, 2020

Conversation

xiaomianhehe
Copy link
Contributor

The array do add_item was slow, use prev pointer to avoid while loop

if (child == NULL)
{
/* list is empty, start new one */
array->child = item;
item->prev = item;
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't the prev of item be NULL? and the default value of item's prev and next item is NULL, so there is no need to point it, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the object is array, we could use the prev pointer, so we can do array_add more quick by "prev".
we don't need to find the last item one by one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the size of array is large , we could do array_add better

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, I just catch your idea, prev way is indeed faster for large array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yayaya, i use it in my project, the performance improve about 30 percent when create a cjson array which has thousand's item.
it could be better, if the element is easier.

cJSON.c Outdated
@@ -1756,7 +1756,7 @@ CJSON_PUBLIC(int) cJSON_GetArraySize(const cJSON *array)
return (int)size;
}

static cJSON* get_array_item(const cJSON *array, size_t index)
static cJSON* get_array_item(const cJSON *array, size_t _index)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not add the _ prefix? I prefer to use index over _index, index is more in line with the naming convention of cJSON

Copy link
Contributor Author

Choose a reason for hiding this comment

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

while compile, it promt the index was a function in string.h
so use _index would be better?

Copy link
Collaborator

Choose a reason for hiding this comment

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

what's your compiler? I have never met this before...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gcc-4.4.7 centios6.5.
we include the string.h file ,and it has a function namd "index",we could modify the compile option, and ignore it, but i don't think it is a good idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i use same compile option,such as Werror, Wall, in my project.

Copy link
Contributor Author

@xiaomianhehe xiaomianhehe Feb 14, 2020

Choose a reason for hiding this comment

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

there are two different c standard. cmakelist.txt configured the option "-std=c89" ,but i use std=gnu99.
I modifid the "std=c89" option to "std=gnu99" in the cmakelist.txt, and it reproduced.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@xiaomianhehe I have modified the std to gnu99, but still no warning. I tested it in macOS, centOS(7.2, gcc4.8.5) and ubuntu(18.04, gcc 7.4.0).
only when string.h has a global varialble named index, the prompt will show.
Have you changed the file of /usr/include/string.h? make sure there is no redundant global variable of index.

  • dup_name.c
#include <stdio.h>
#include <string.h>
#include "lib1.h" // when including lib1.h, it failed.

void func(int index) {
    printf("index = %d\n", index);
}

int main() {
    func(5); // print: index = 5
    char *str = "hello world";
    char *res = index(str, 'o');
    printf("%s\n", res);   // print: o world
    return 0;
}
  • lib1.h
int index = 0;

As far as I know, method parameter of local variable named index is very common. such as jsoncpp, you could try to compile it and above example code.

After this issue is resolved, I will merge your PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. i try gcc-4.4.7(Centos6.5)and gcc-4.8.5(centos7.2), the result was different.
it has warning with gcc-4.4.7.

Copy link
Collaborator

Choose a reason for hiding this comment

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

so I think there is no need to change index, do you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I revert it.I modify it in my project, because we use old version gcc and centos in our produce env.

Copy link
Collaborator

@Alanscut Alanscut left a comment

Choose a reason for hiding this comment

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

please revert _index to index

@xiaomianhehe
Copy link
Contributor Author

Done.

@Alanscut Alanscut merged commit e8077d0 into DaveGamble:master Feb 18, 2020
@Alanscut
Copy link
Collaborator

Thanks!

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