Clean up the cJSON struct

[FSMaxB]

Currently theres three ways the cJSON struct can be improved:

1. Get rid of valueint
2. Use a union for valuestring and valuedouble
3. Using good names for the elements

If you don't think this is a good idea, please tell me why.

Let me explain:

Getting rid of valueint

• Having both valueint and valuedouble introduces unnecessary complexity in cJSON.
• Having both valueint and valuedouble increases the chance of misuse by users of the library
• With an IEEE754 compliant floating point implementation, valueint is not bringing any extra value on platforms that have integers smaller than 32 bit
  • double can store integers up to 53 bits (> 32)
  • even if you have a 64 bit integer, this code prevents it from being useful, because it only works if the integer fits into the double type completely.
  • that means, that even on a floating point implementation that is not IEEE754 compliant, the range of supported numbers is completely bounded by the implementation of double, cJSON will never print an exact value of an integer bigger than the biggest exact integer that can be represented by double
• The valueint is using between 2-8 bytes that are completely wasted
  • it might be even more because of alignment, if we have a processor that requires 8 byte alignment and has a 64 bit pointer type and a 32 bit integer type, the struct will look like this
    • next: 8 byte
    • prev 8 byte
    • child 8 byte
    • type 4 byte
    • 4 byte padding
    • valuestring 8 byte
    • valueint 4 byte
    • 4 byte padding
    • valuedouble 8 byte
    • string 8 byte
    • even though valueint has only 4 bytes, it takes up 8 bytes in the struct

Using a union of valuestring and valuedouble

As an instance of the cJSON struct can only be either a string type or a number type, not both at the same time. Therefore they can be stored at the same place in memory.

New version of the cJSON struct

typedef struct cJSON
{
    /* next/prev allow you to walk array/object chains. Alternatively, use GetArraySize/GetArrayItem/GetObjectItem */
    struct cJSON *next;
    struct cJSON *previous;
    /* An array or object item will have a child pointer pointing to a chain of the items in the array/object. */
    struct cJSON *child;
    /* The item's name string, if this item is the child of, or is in the list of subitems of an object. */
    char *name;
    union {
        /* The item's string, if type==cJSON_String */
        char *string;
        /* The item's number, if type==cJSON_Number */
        double number;
    }
    /* The type of the item, as above. */
    int type;
} cJSON;

• renamed prev -> previous
• valueint removed
• renamed valuedouble -> number
• renamed string -> name
• renamed valuestring -> string
• put string and number in a union
• reorder the elements in the struct

[DaveGamble]

With fairly minimal rework, one could eliminate previous too. It's barely ever read in the library, and when it is, a single temp variable could eliminate it.

[FSMaxB]

Yes, now that I think about it the library only ever uses it as single linked list.

[iMobs]

You might also want to play around with the order of the variables for better struct packing. Depending on the word/pointer size of the system, this could reduce the size of the object.

[FSMaxB]

There are potentially 3 different sizes in the new struct. The size of a pointer, sizeof(int) and sizeof(double). The order I used in the suggested version of the new struct already optimizes that for x86_64 with still being the best version for 32 bit x86. But I haven't considered all the other possible size combinations on other processors.

This would change when removing the previous pointer though.

[FSMaxB]

With previous removed, the following variant has no padding on all architectures I can think of:

typedef struct cJSON
{
    union {
        char *string;
        double number;
    }
    char *name;
    struct cJSON *next;
    struct cJSON *child;
    int type;
} cJSON;

[ffontaine]

I would suggest to give a name to the union such as data or u indeed anonymous structures and unions are a C11 feature.

[FSMaxB]

I consider this C11 compatible (if even) pseudo code XD. That shows you that I never put this code through a compiler. But correct syntax doesn't matter for the purpose of this discussion.

[ffontaine]

Sorry, I think my comment was incomplete and didn't give you enough background. I wanted to point out that I would like to keep cjson compatible with C99 and to avoid any C11 features such as anonymous structures. Indeed some old gcc like gcc 4.3 (which is used to compile cjson for blackfin target on buildroot) will not be able to compile cjson anymore.

[FSMaxB]

Actually, cJSON is keeping compatible with C89 and avoids any C99 and C11 features (that haven't been available in C89). Don't worry, this GitHub issue is not cJSONs source code!

[Dreaster]

How about changing the type to be an enum instead so that the the code will be clearer?

[FSMaxB]

That sounds like a really good idea, but I thought that it is undefined behavior to use an enum with another than the predefined values. Let me check if that is actually true.

Because type is supposed to be used as bit flags, so you can check if something is array or object for example (e.g. if (json->type & (cJSON_Array | cJSON_Object)).

[Dreaster]

You are very correct. I have completly missed that the type was used as bit flags. I thought that it truly was one of the cJSON_types defines. In that case. A different name perhaps inroder to clairify. "used_types" or something like that.