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.

Sign up for GitHub

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

Fixing all /W4 level warnings. #139

Merged
merged 3 commits into from
Mar 30, 2017
Merged

Fixing all /W4 level warnings. #139

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
16e22d6
Fixing all W4-level warnings.
PawelWMS Mar 29, 2017
2ea410c
Adding back "const".
PawelWMS Mar 30, 2017
31f0fff
Renaming "min" to "cjson_min".
PawelWMS Mar 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions cJSON.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,21 @@
/* cJSON */
/* JSON parser in C. */

#ifdef __GNUC__
#pragma GCC visibility push(default)
#endif

#include <string.h>
#include <stdio.h>
#include <math.h>
#include <stdlib.h>
#include <float.h>
#include <limits.h>
#include <ctype.h>

#ifdef __GNUC__
#pragma GCC visibility pop
#endif

#include "cJSON.h"

Expand Down Expand Up @@ -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 };
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@PawelWMS PawelWMS Mar 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just checked. GCC is not optimizing out the memset. In case { 0 } is invalid, replace it with { 0, 0, 0}.

Copy link
Contributor Author

@PawelWMS PawelWMS Mar 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I agree that this means that all of them are 0, so no changes required here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like I was a little too fast: https://travis-ci.org/FSMaxB/cJSON/jobs/216977810, I will fix it in another commit.

cJSON *item = NULL;

/* reset error position */
Expand Down Expand Up @@ -1019,7 +1025,9 @@ CJSON_PUBLIC(cJSON *) cJSON_Parse(const char *value)
return cJSON_ParseWithOpts(value, 0, 0);
}

#ifndef min
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They do. It's inside stdlib.h

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please rename to cjson_min

#define min(a, b) ((a < b) ? a : b)
#endif

static unsigned char *print(const cJSON * const item, cJSON_bool format, const internal_hooks * const hooks)
{
Expand Down Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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 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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it.

{
cJSON *current_element = NULL;

Expand Down Expand Up @@ -1750,7 +1758,10 @@ CJSON_PUBLIC(void) cJSON_AddItemToObject(cJSON *object, const char *string, cJSO
#if defined (__clang__) || ((__GNUC__) && ((__GNUC__ > 4) || ((__GNUC__ == 4) && (__GNUC_MINOR__ > 5))))
#pragma GCC diagnostic push
#endif
#ifdef __GNUC__
#pragma GCC diagnostic ignored "-Wcast-qual"
#endif

/* Add an item to an object with constant string as key */
CJSON_PUBLIC(void) cJSON_AddItemToObjectCS(cJSON *object, const char *string, cJSON *item)
{
Expand Down