• Advertisement
Sign in to follow this  

Duplication?

This topic is 3316 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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|

Share this post


Link to post
Share on other sites
Advertisement

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.

Share this post


Link to post
Share on other sites
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|

Share this post


Link to post
Share on other sites
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)?

Share this post


Link to post
Share on other sites
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).

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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;
}

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
char *task_serialize(Task *task)
{
// Add one for the camma
int size = strlen(task->task);
size += strlen(task->project);
size += strlen(task->context);
// two for priority
size += 1;
// two for done bool
size += 1;
// two for nextAction bool and cammas
size += 8;

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;
}


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|

Problem with my parsing code?

Share this post


Link to post
Share on other sites
This is the value of a free standing, compilable example.

This Works For MeTM.

(note: minor modifications made to compile under a c++ compiler).

#include <cstdio>
#include <cstring>
#include <cstdlib>

typedef enum {NORMAL, LOW, MEDIUM, HIGH} priority;

typedef struct
{
char *task;
char *project;
char *context;
priority priority;
bool done;
bool nextAction;
} Task;

Task *make_task(const char *task,const char *project,const char *context,priority p,bool done,bool nextAction)
{
Task *t = (Task *)malloc(sizeof(Task));
t->task = _strdup(task);
t->project = _strdup(project);
t->context = _strdup(context);
t->priority = p;
t->done = done;
t->nextAction = nextAction;
return t;
}

void free_task(Task *t)
{
if(t)
{
free(t->task);
free(t->project);
free(t->context);
free(t);
}
}

const char *task_serialize(Task *task)
{
// Add one for the camma
int size = strlen(task->task);
size += strlen(task->project);
size += strlen(task->context);
// two for priority
size += 1;
// two for done bool
size += 1;
// two for nextAction bool and cammas

size += 8;

char *result = (char *)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;
}

int main()
{
Task *task = make_task("Check email","none","computer",NORMAL,1,1);

const char *string = task_serialize(task);

printf("%s\n",string);

free_task(task);
}


Output:
Quote:

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

Share this post


Link to post
Share on other sites
Actually in your code I don't see where you are doing anything to data. Aren't all these lines finding the same , character?


i = strchr(data, ',');

Share this post


Link to post
Share on other sites
Hmm you're right. After I read the data upto the first comma, is there a way to cut that out? It didn't like me copyring the string from data back into data using strcpy.

ripoff,load_task is ment to load a line from a txt file. So I think my problem is in the code parsing, as visitor said.

Share this post


Link to post
Share on other sites
I presume I will just have to coppy it to a new array, everytime I finish reading a part of the data string.

Unless what I do is create a pointer to a character, then just keep free()ing the number of characters I don't need anymore.

Share this post


Link to post
Share on other sites
So I discovered strtok(), but every time I use it, it returns a stinking bus error. This is getting on my nerves. Maybe I'll switch back to my comfy cozy C++.

Share this post


Link to post
Share on other sites
strtok modifies the input string (inserts '\0' at the beginning of delimiter series / end of token). So if you happen to pass in a string literal (which is unmodifyable) you can get errors since the string literals often are in unmodifyable memory. Whenever a function accepts a literal string, the argument must be const (const char*), then the compiler would tell you (hopefully, in C const errors don't seem to always be compiler errors...).

Perhaps, what you might do is make a copy of the input string (allocated dynamically and added as a struct member) and the rest of the struct members simply point at tokens in this string (no additional memory allocation required).

Of course, if you don't need to do this in C, C++ might have more convenient tools for tokenizing and concatenation.

Share this post


Link to post
Share on other sites
Your parsing code has problems as well.

For one, you copy and paste code. Don't do that. Write a helper function to "copy from the string up to the next comma". This, in turn, can make use of standard C library functions.

For another, you set the Task object's members to point at the buffers you've malloc()'ed, and then immediately free() them. This is bad. The pointers are now immediately invalid. The whole point of free() is to say "I don't need the pointed-at thing any more". This is clearly not what you intend. That's only appropriate for reading the integer values, where you throw away the string after getting the integer.


/* EDIT: Expecting to be able to modify the input is a needless restriction;
thanks to visitor for pointing out the value of avoiding that. */

/* EDIT 2: shortened logic, taking full advantage of my clever idea to copy
the pointer. ;) */


char *next_item(char **begin_ptr) {
/* before: *begin_ptr points at a string (possibly in the middle).
after: *begin_ptr points just past the next comma, and the return value is
a newly allocated string representing everything up to the first
comma.
Repeatedly calling this, therefore, will extract each comma-delimited
item of the string, as *begin is updated to the appropriate place each
time. (You could also loop while the pointer is non-NULL.)
*/

char *result, *begin = *begin_ptr, end;

if (!begin) return NULL;

/* Find the comma, if any, and update the out-parameter to point past it
(or set to NULL if there is no comma). 'begin' is untouched by this */

end = strchr(begin, ',');
if (end) {
*begin_ptr = end + 1;
} else {
*begin_ptr = NULL;
end = begin + strlen(begin);
}

/* Copy the appropriate string data and return it. */
result = malloc(end - begin);
strncpy(result, begin, end - begin);

return result;
}

int next_item_as_int(char **begin_ptr) {
int result;
char *result_str = next_item(begin_ptr);

if (!result_str) return 0;
result = atoi(result_str);
free(result_str);
return result;
}

Task *load_task(char *data) {
/* BTW, C89 doesn't allow you to declare variables in the middle of a scope
(at least if the compiler is strict), although C99 does. */

Task *result = malloc(sizeof(Task));
char *priority_str, *done_str, *next_str;

result->task = next_item(&data);
result->project = next_item(&data);
result->context = next_item(&data);
result->priority = next_item_as_int(&data);
result->done = next_item_as_int(&data);
result->nextAction = next_item_as_int(&data);

return result;
}






Hopefully this gives you a few good ideas about how to organize code, particularly as concerns memory management. Many of these ideas apply to C++ as well, although in C++ you have a lot of other more powerful tools too.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement