Menu

#43 cJSON_Delete() unwanted behaviour

v1.0 (example)
closed-rejected
None
6
2015-02-14
2015-02-13
a_teammate
No

Hi, my second try now to point to the unwanted behaviour ( a so called bug) in your code:

void cJSON_Delete(cJSON c)
{
cJSON
next;
while (c)
{
next=c->next;
if (!(c->type&cJSON_IsReference) && c->child) cJSON_Delete(c->child);
if (!(c->type&cJSON_IsReference) && c->valuestring) cJSON_free(c->valuestring);
if (!(c->type&cJSON_StringIsConst) && c->string) cJSON_free(c->string);
cJSON_free(c);
c=next;
}
}>

This time I'll explain it more precisly why this is wrong and I will even provide you a patch, since I see you either do not remember the code anymore yourself (not to judge imo, i see its originally written in 2009) or do not care and do not want to care.

What your code does (as i wrote in my first bug report already):
It deletes c plus deletes every of its following neighbors.

What it should do is:
It deletes c.

(There ya go, i explained it nice and simple, instead of vague as in my first bug report, what may have caused your irritation)

patch:

void cJSON_Delete(cJSON *c)
{
if (!(c->type&cJSON_IsReference) && c->valuestring) cJSON_free(c->valuestring);
if (!(c->type&cJSON_StringIsConst) && c->string) cJSON_free(c->string);

    cJSON *ch = c->child;
    if(!(c->type&cJSON_IsReference)) while (ch)
    {
            cJSON *bak = ch->next;
            cJSON_Delete(ch);
            ch=bak;
    }
    cJSON_free(c);

}

off topic from now on:
Another point I want to mention is that "bug reports" are nothing you should be angry about, they build an important base for open-source projects and show that others care about the code and want to improve it.
As i see you are a one-man-show, but I'll tell you that one day you find a project too big to be accomplished by just yourself and you'll have to learn to act in a team.

Btw, in open-source projects, one "closes" bug-reports, instead of deleting it. You still can add tags to show that it is not a bug (e.g. 'wont-fix').

And to the closing part of your rather funny than actual insulting response (although that was actually its purpose i guess)
"This library is not designed for you. Please find another."

I will, thank you. I already rewrote this entire Library in object orientated c++ for an open-source game I'm contributing to.

Happy Valentine's

Discussion

  • Dave Gamble

    Dave Gamble - 2015-02-14

    Thanks for your clear exposition of the issue.

    What you've found is an implementation detail that while unexpected, is intentional. The issue stems from whether or not, given the tree structure of cJSON structs, it is meaningful to delete an array entry or object member independently.

    The correct usage is to detach an array/object member before deletion, or to delete it using DeleteFromArray or DeleteFromObject.

    From your exposition, I can see that this was not clear to you.
    I'm not keen to change the current behaviour, because the library is used by a large number of projects, and anyone relying on the current behaviour (which allows you to delete a chain in one go) would be in for a nasty shock (read: memory leaks).

    For what it may count, I do care, and the project is still maintained. I deleted your previous report because it didn't contain a clear report of the bug, and as such wasn't suitable for closing as won't-fix.

    I'm always happy to receive bug reports delivered as such, but I'm afraid I need reports of the quality you've provided here to take action.

    Your off-topic rebuke is graciously received, and I'm glad to hear that you have a working solution.

     
  • Dave Gamble

    Dave Gamble - 2015-02-14
    • status: unread --> closed-rejected
     

Log in to post a comment.

MongoDB Logo MongoDB