This is wrong:
Code:
void initList(list_t *list)
{
list = (list_t*)malloc(sizeof(list_t));
list->head = NULL;
list->size = 0;
}
The problem is that you are assigning the result of malloc to a local variable whose lifetime is that of the function, but you don't return its value, so you end up with a memory leak. There are two general approaches here:
Code:
list_t *createList(void)
{
list_t *list = malloc(sizeof(*list));
if (list)
{
list->head = NULL;
list->size = 0;
}
return list;
}
In the above code, I choose to return the pointer, so I renamed the function to createList. Notice that I check the return value of malloc before using it, and that this returns a null pointer in the unlikely situation that the list could not be created. We would expect createList to be used like this:
Code:
list_t *list = createList();
if (!list)
{
// Handle memory allocation failure, or rage quit
// ...
}
// Code that uses the linked list
// ...
If you really prefer to initialise the linked list instead:
Code:
void initList(list_t **list)
{
list_t *temp = malloc(sizeof(*temp));
if (temp)
{
temp->head = NULL;
temp->size = 0;
}
*list = temp;
}
You can see that the parameter is a pointer to a pointer instead. This way, the list_t pointer in the caller can be modified from within the function. I have kept to the same pattern as with createList by making use of the temp local variable, but if you prefer you could use *list instead. We would expect initList to be used like this:
Code:
list_t *list = NULL;
initList(&list);
if (!list)
{
// Handle memory allocation failure, or rage quit
// ...
}
// Code that uses the linked list
// ...
Notice that for both createList and initList, the caller (i.e., the main function in your case) declare list to be a pointer. If you really want list to be a list_t object instead, then you would not use malloc within initList. Rather:
Code:
void initList(list_t *list)
{
list->head = NULL;
list->size = 0;
}
As you can see, this is almost the code that you originally wrote, but with the malloc line deleted. If you do it this way, then your original idea will work:
Code:
list_t list;
initList(&list);
// Code that uses the linked list
// ...
Next, I don't recommend this:
Code:
int insert(list_t *list, node_t * node)
{
node_t * pointer = node;
pointer = malloc(sizeof(node_t));
pointer->value = node->value;
pointer->next = list->head;
list->size += 1;
}
You're basically using the node parameter as a way to pass the value. Why do that when you can just pass the value? For example:
Code:
int insert(list_t *list, int value)
{
node_t *node = malloc(sizeof(*node));
if (!node)
{
return 0;
}
node->value = value;
node->next = list->head;
list->head = node;
list->size += 1;
return 1;
}
Notice that I am making use of the int return value as a boolean value, i.e., true for success, false for failure. You can consider #include <stdbool.h> to use bool, true, and false instead of int, 1, and 0. Notice also that I take care to assign the new node as the new head of the linked list, otherwise you'll be incrementing the size, but your linked list won't actually grow.
Oh, and remember to free what you malloc, e.g., by writing a destroyList or freeList function and calling it. It doesn't matter for such a short-lived program, but it will be good practice.
Originally Posted by Bjarne Stroustrup (2000-10-14)
