Fixing all /W4 level warnings. #139
Conversation
cJSON.c
Outdated
| @@ -1666,7 +1674,7 @@ CJSON_PUBLIC(cJSON *) cJSON_GetObjectItem(const cJSON *object, const char *strin | |||
| return c; | |||
| } | |||
|
|
|||
| CJSON_PUBLIC(cJSON *) cJSON_GetObjectItemCaseSensitive(const cJSON * const object, const char * const string) | |||
| CJSON_PUBLIC(cJSON *) cJSON_GetObjectItemCaseSensitive(const cJSON * object, const char * string) | |||
There was a problem hiding this comment.
Please don't remove the const's here, rather add them to the header file as well. It is a bug that they aren't in the header.
There was a problem hiding this comment.
I see. I'm curious, though - why is it important for the pointer to be constant? The pointer itself is already passed by value and any manipulations on it won't influence the caller.
There was a problem hiding this comment.
The object pointer is assumed to never change, so the compiler can as well enforce it.
It is good practice to keep everything const as much as possible if you are not going to change it. It can help the compiler with telling you that you're doing something wrong.
In this case I could have not used current_element and just used object all the time, but this way the intent is clear.
| @@ -935,7 +941,7 @@ static parse_buffer *buffer_skip_whitespace(parse_buffer * const buffer) | |||
| /* Parse an object - create a new root, and populate. */ | |||
| CJSON_PUBLIC(cJSON *) cJSON_ParseWithOpts(const char *value, const char **return_parse_end, cJSON_bool require_null_terminated) | |||
| { | |||
| parse_buffer buffer; | |||
| parse_buffer buffer = { 0 }; | |||
There was a problem hiding this comment.
What does this even do? Is this specified in the C89 standard? I might have to look into this.
Note that all elements of the struct are initialized before use.
What exactly is the compiler complaining about?
There was a problem hiding this comment.
They are not initialized if "value == NULL" below is true and we skip right to "fail". We could also initialize each member of the struct explicitly or use memset(), so that the code will automatically follow any changes to the struct.
There was a problem hiding this comment.
But it's never accessed if value is NULL. Whatever I still want the code to compile cleanly with major compilers, even if there is a false positive.
I still wonder about the { 0 } initializer. I that one is valid C89 and equivalent to setting all elements to 0, then OK, otherwise memset is acceptable, as long as it gets optimized out.
There was a problem hiding this comment.
I just checked. GCC is not optimizing out the memset. In case { 0 } is invalid, replace it with { 0, 0, 0}.
There was a problem hiding this comment.
You're right - the compiler isn't smart enough to figure this out.
From what I was able to find here { 0 } should initialize all members to zero:
If there are fewer initializers in a list than there are members of an aggregate, the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration.
And an object with a static duration is initialized this way:
If an object that has static storage duration is not initialized explicitly, it is initialized implicitly as if every member that has arithmetic type were assigned 0 and every member that has pointer type were assigned a null pointer constant.
There was a problem hiding this comment.
Yes I agree that this means that all of them are 0, so no changes required here.
There was a problem hiding this comment.
Seems like I was a little too fast: https://travis-ci.org/FSMaxB/cJSON/jobs/216977810, I will fix it in another commit.
cJSON.c
Outdated
| @@ -1019,7 +1025,9 @@ CJSON_PUBLIC(cJSON *) cJSON_Parse(const char *value) | |||
| return cJSON_ParseWithOpts(value, 0, 0); | |||
| } | |||
|
|
|||
| #ifndef min | |||
There was a problem hiding this comment.
Where would this min define come from? Do the Microsoft headers contain such a macro?
If there is a conflict it might be better to rename it to cjson_min or just inline it. Although inlining will make the code less readable.
There was a problem hiding this comment.
They do. It's inside stdlib.h
There was a problem hiding this comment.
Please rename to cjson_min
A small change removing all /W4 level warnings when cJSON is built using a Visual Studio 2013 or 2015 compiler.