cJSON icon indicating copy to clipboard operation
cJSON copied to clipboard

Feature Request: Merge valuestring, valueint and valuedouble to union

Open NehCoy opened this issue 4 years ago • 2 comments

Hello!

Just an idea: Why didn't you merge the valuestring, valueint and valuedouble parameters of the cJSON structure in an union?

/* The cJSON structure: */
typedef struct cJSON
{
    /* next/prev allow you to walk array/object chains. Alternatively, use GetArraySize/GetArrayItem/GetObjectItem */
    struct cJSON *next;
    struct cJSON *prev;
    /* 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 type of the item, as above. */
    int type;

    union
    {
        /* The item's string, if type==cJSON_String  and type == cJSON_Raw */
        char * s;
        /* writing to valueint is DEPRECATED, use cJSON_SetNumberValue instead */
        int32_t i32;
        int64_t i64; /* support of #566  */
        /* The item's number, if type==cJSON_Number */
        double d;
    } value;

    /* The item's name string, if this item is the child of, or is in the list of subitems of an object. */
    char *string;
} cJSON;

For sure, it's not backward compatible. - However, it's reducing the size of the cJSON structure. In addition of that I prefer the access like object->value.i32.

Best regards, NC

NehCoy avatar Jul 09 '21 14:07 NehCoy

I had almost the same thought as I browsed through the source code.

In addition to what you proposed here, I also thought putting the child field into the union, because cJSON either represents an atomic datatype (string, number, bool) or a composite one (object, array), and the child field is reserved for the latter (think of it as the "value" of the object or array).

So something like the following:

/* The cJSON structure: */
typedef struct cJSON
{
    /* next/prev allow you to walk array/object chains. Alternatively, use GetArraySize/GetArrayItem/GetObjectItem */
    struct cJSON *next;
    struct cJSON *prev;

    /* The type of the item, as above. */
    int type;

    union
    {
        /* The item's string, if type==cJSON_String  and type == cJSON_Raw */
        char * s;
        /* writing to valueint is DEPRECATED, use cJSON_SetNumberValue instead */
        int32_t i32;
        int64_t i64; /* support of #566  */
        /* The item's number, if type==cJSON_Number */
        double d;
        /* An array or object item will have a child pointer pointing to a chain of the items in the array/object. */
        struct cJSON *child;
    } value;

    /* The item's name string, if this item is the child of, or is in the list of subitems of an object. */
    char *string;
} cJSON;

StrongerXi avatar Jan 16 '22 16:01 StrongerXi

Defines might make it easier to reach into the union with less changed.

Anonymous unions would be really nice for this, but the codebase insists on being C89 clean, so...

Note that this is weird with the parallel storage of int & double values that the code depends upon elsewhere. You really need to pick one canonical representation to use a union like this-- and this would be good for the rest of the code quality, too.

mlyle avatar Jan 17 '22 06:01 mlyle