Back to General and Gameplay Programming

Duplication?

General and Gameplay Programming Programming

Started by Stormtrooper December 21, 2008 01:38 AM

16 comments, last by Zahlman 15 years, 4 months ago

Stormtrooper

100

Author

December 21, 2008 01:38 AM

I have this(albeit messy) code that parses a string and fills a struct with the data. However, I get weird results.

int main(int argc, char **argv)
{
 
    Task *task = load_task("Check email,none,computer,0,1,1|");
    
    printf("%s\n", task_serialize(task));
    free(task);
    return 0;
}


char *task_serialize(Task *task)
{
    char *result = malloc(sizeof(task)); 
    sprintf( result, "%s,%s,%s,%i,%i,%i|",
            task->task,
            task->project,
            task->context,
            task->priority,
            task->done,
            task->nextAction);
                                                        
    return result;
}

Task *load_task(char *data)
{
    Task *result = malloc(sizeof(Task));
    
    // First get the task description
    char *i = strchr(data, ',');
    int in = i - data;
    char *task = malloc(in);
    strncpy(task, data, in - 1);
    
    result->task = task;
    free(task);
    
    // Remove the task desc.
    //strncpy(data, data + in, strlen(data));
    
    // Get the task project
    i = strchr(data, ',');
    in = in + i - data;
    char *project = malloc(in);
    strncpy(project, data, in - 1);
    
    result->project = project;
    free(project);
    
    // Remove the task desc.
    //strncpy(data, data + in, strlen(data));
    
    // Get the context
    i = strchr(data, ',');
    in = in + i - data;
    char *context = malloc(in);
    strncpy(context, data, in - 1);
    
    result->context = context;
    free(context);
    
    // Remove the task desc.
    //strncpy(data, data + in, strlen(data));
    
    // Get the priority
    i = strchr(data, ',');
    in = in + i - data;
    char *priority = malloc(in);
    strncpy(priority, data, in - 1);
    
    result->priority = atoi(priority);
    free(priority); 
    
    // Remove the task desc.
    //strncpy(data, data + in, strlen(data));
    
    // Get if the task is done
    i = strchr(data, ',');
    in = in + i - data;
    char *done = malloc(in);
    strncpy(done, data, in - 1);
    
    result->done = atoi(done);
    free(done);
    
    // Remove the task desc.
    //strncpy(data, data + in, strlen(data));
    
    // Get if the task is next action
    i = strchr(data, ',');
    in = in + i - data;
    char *nextaction = malloc(in);
    strncpy(nextaction, data, in - 1);
    
    result->nextAction = atoi(nextaction);
    free(nextaction);
    
    return result;
}

I get this weird output: Check email,none,computer,0,1,1|,,computer,0,1,1|,,0,0,0| It should be what I inputted: Check email,none,computer,0,1,1|

[Broken Silence] [GameDev Wiki]

visitor

643

December 21, 2008 05:58 AM

char *task_serialize(Task *task){    char *result = malloc(sizeof(task));

It seems that you allocate only 4 (8) bytes for result (sizeof pointer). If the Task struct itself contains char pointers I would guess it is impossible to know how much memory you'll need unless the task object itself knows it (the total length of string fields + separators + null terminator).

    char *task = malloc(in);    strncpy(task, data, in - 1);        result->task = task;    free(task);

You allocate memory, put some data in it, assign the pointer to a struct member and immediately deallocate the memory, effectively rendering the pointer within the Task struct invalid.

Stormtrooper

100

Author

December 22, 2008 11:37 AM

So I tried this to get a better estimate for the size:

    int size = strlen(task);    size += strlen(project);    size += strlen(context);    // one for priority    size += 1;    // one for done bool    size += 1;    // one for nextAction bool    size += 1;    Task *result = malloc(size);        result->task = task;    result->project = project;    result->context = context;    result->priority = atoi(priority);    result->done = atoi(done);    result->nextAction = atoi(nextaction);

And got this output:
Check emai,Check email,none,comp,Check email,none,computer,0,1,1|,0,0,0|

should be
Check email,none,computer,0,1,1|

[Broken Silence] [GameDev Wiki]

visitor

643

December 22, 2008 12:25 PM

Are you also taking into account that strings need one extra character worth of space for the null-terminator?

Could you post a full compilable example (the two functions, the struct definition and main - together with includes)?

Stormtrooper

100

Author

December 22, 2008 01:04 PM

Here (zip)

[Broken Silence] [GameDev Wiki]

rip-off

11,000

December 22, 2008 01:41 PM

To allocate an instance of task struct, you could use something like this:

Task *make_task(const char *task,const char *project,const char *context,priority p,bool done,bool nextAction){    Task *t = malloc(sizeof(Task));    t->task = strdup(task);    t->project = strdup(project);    t->context = strdup(context);    t->priority = p;    t->done = done;    t->nextAction = nextAction;}void free_task(Task *t){    if(t)    {        free(t->task);        free(t->project);        free(t->context);        free(t);    }}

The free_task() assumes that the task is always allocated with make_task(). Note that we have 4 calls to free(). This matches with the number of allocations, 1 to malloc() and 3 to strdup() (which is documented as returning a pointer that can be deallocated with a call to free).

blog | twitter

visitor

643

December 22, 2008 01:54 PM

Well, one more time...

char *task_serialize(Task *task){    char *result = malloc(sizeof(task));...

What do you think sizeof(task) is and is it enough to hold all the data in the Task object? (Note, sizeof(*task) wouldn't help you either.)

Another very similar thing

    char *result = malloc(sizeof(getCwd()));    sprintf(result, "%s/", getCwd());

Do you know what is the difference between sizeof and strlen? sizeof is something that evaluates size of stuff at compile-time (a char* in this case), it is impossible that it would give you the length of a string that is known only at run-time.

It might still be better to provide a minimal example, just something that can be easily compiled without additional dependencies.

Stormtrooper

100

Author

December 22, 2008 02:04 PM

Okay, I understand what you are saying. Shouldn't this work?

char *task_serialize(Task *task){    int size = strlen(task->task);    size += strlen(task->project);    size += strlen(task->context);    // one for priority    size += 1;    // one for done bool    size += 1;    // one for nextAction bool and null termination    size += 2;        char *result = malloc(size);        //memcpy((void *)result, (void *)task, sizeof(task));    sprintf( result, "%s,%s,%s,%i,%i,%i|",            task->task,            task->project,            task->context,            task->priority,            task->done,            task->nextAction);                                                            return result;}

[Broken Silence] [GameDev Wiki]

visitor

643

December 22, 2008 02:08 PM

Do you also leave room for the commas?

rip-off

11,000

December 22, 2008 02:16 PM

Hmm, I misread what you were trying to do. I think writing directly to a file would be easier, because that is what the calling code inevitably appears to be doing:

void task_serialize(Task *task, FILE *dest);

At the moment it appears that your code doesn't take into account the commas and the pipe character that is in the format string.

blog | twitter

Duplication?

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Duplication?

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines