A few issues ...
- In
LIST, items should be just void * and not void **
- In
list_add, memory is a stack local variable that goes out of scope. Change list->items = &memory to list->items = memory
- You just use
list->count + 1 in the realloc. We want to increment list->size instead and use that in the call.
- The actual store code (in the
else block) should be executed unconditionally
- The storing code tries to dereference a
void * pointer
- Because the list is storing strings (i.e.
char * arrays), we need to duplicate the string when storing. So, we should use strdup
- Because of the new
strdup, just doing free(list) at the end is insufficient. We need to loop through all elements and free the strings we duplicated in list_add.
list_init is replicating code in list_add. It should just do the first calloc only.- Because we can pass in string literals to
list_add (e.g. list_add(list,"Hello");, the 2nd arg should have const.
Here is the refactored code. It is annotated with the bugs and fixes:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
struct LIST {
unsigned int size;
unsigned int count;
#if 0
void **items;
#else
void *items;
#endif
};
struct LIST *
list_init(unsigned int size)
{
#if 0
// NOTE/BUG: do _not cast the return of calloc (or malloc/realloc)
struct LIST *createList = (struct LIST *) calloc(1, sizeof(struct LIST));
#else
struct LIST *createList = calloc(1, sizeof(*createList));
#endif
#if 0
// NOTE/BUG: this replicates code in list_add
void *memory = calloc(size, sizeof(void *));
if (memory == 0 || createList == 0)
return 0;
createList->items = &memory;
createList->size = size;
#endif
return createList;
}
#if 0
// NOTE/BUG: this should be a const pointer because we can pass in a string
// literal
void
list_add(struct LIST *list, void *item)
#else
void
list_add(struct LIST *list, const void *item)
#endif
{
if (list->count + 1 > list->size) {
#if 0
// NOTE/BUG: we need to increment list->size
void *memory = realloc(list->items, (sizeof(void *)) * list->count + 1);
#else
list->size += 3;
void *memory = realloc(list->items, (sizeof(void *)) * list->size);
#endif
#if 0
// NOTE/BUG: the variable 'memory' is on the stack and goes out of scope
list->items = &memory;
return;
#else
list->items = memory;
#endif
}
#if 0
// NOTE/BUG: this should be executed unconditionally
// NOTE/BUG: we need to duplicate the 'item' string
// NOTE/BUG: we can _not_ dereference a void * pointer
else {
list->items[list->count] = item;
list->count = list->count + 1;
}
#else
char **items = list->items;
items[list->count] = strdup(item);
list->count = list->count + 1;
#endif
}
#if 1
void
list_print(struct LIST *list)
{
printf("Items in List: %d\n", list->count);
const char **items = list->items;
if (items != NULL) {
for (unsigned int idx = 0; idx < list->count; ++idx, ++items)
printf("Item: %s\n",*items);
}
}
void
list_free(struct LIST *list)
{
char **items = list->items;
if (items != NULL) {
for (unsigned int idx = 0; idx < list->count; ++idx, ++items)
free(*items);
}
free(list);
}
#endif
int
main(void)
{
const char *str = "Hello";
struct LIST *list = list_init(2);
#if 0
// NOTE/BUG: no need to cast str and can be detrimental as in masks the
// const nature of a string literal as below
list_add(list, (void *) str);
list_add(list, (void *) str);
list_add(list, (void *) str);
list_add(list, (void *) str);
#else
list_add(list, str);
list_add(list, "abc");
list_add(list, "def");
list_add(list, "ghi");
list_add(list, "jkl");
#endif
#if 0
printf("Items in List: %d \n\n", list_count(list));
list_clear(list);
#else
list_print(list);
#endif
#if 0
free(list);
#else
list_free(list);
#endif
return 0;
}
In the code above, I've used cpp conditionals to denote old vs. new code:
#if 0
// old code
#else
// new code
#endif
#if 1
// new code
#endif
Note: this can be cleaned up by running the file through unifdef -k
Here is the program output:
Items in List: 5
Item: Hello
Item: abc
Item: def
Item: ghi
Item: jkl
Here is the cleaned up code:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
struct LIST {
unsigned int size;
unsigned int count;
void *items;
};
struct LIST *
list_init(unsigned int size)
{
struct LIST *createList = calloc(1, sizeof(*createList));
return createList;
}
void
list_add(struct LIST *list, const void *item)
{
if (list->count + 1 > list->size) {
list->size += 3;
void *memory = realloc(list->items, (sizeof(void *)) * list->size);
list->items = memory;
}
char **items = list->items;
items[list->count] = strdup(item);
list->count = list->count + 1;
}
void
list_print(struct LIST *list)
{
printf("Items in List: %d\n", list->count);
const char **items = list->items;
if (items != NULL) {
for (unsigned int idx = 0; idx < list->count; ++idx, ++items)
printf("Item: %s\n",*items);
}
}
void
list_free(struct LIST *list)
{
char **items = list->items;
if (items != NULL) {
for (unsigned int idx = 0; idx < list->count; ++idx, ++items)
free(*items);
}
free(list);
}
int
main(void)
{
const char *str = "Hello";
struct LIST *list = list_init(2);
list_add(list, str);
list_add(list, "abc");
list_add(list, "def");
list_add(list, "ghi");
list_add(list, "jkl");
list_print(list);
list_free(list);
return 0;
}
