Generic stack implementation in C

Question

I haven't separated the stack code into its own header and source yet, this is just a proof of concept for now. Haven't chosen better function names yet because I ultimately want to integrate this code into a larger project where I need a way to store data into some kind of dynamic list. Settled for an stack API for now, because of the simplicity.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
struct CodeGroup
{
 char name[256];
 struct GenericNode *codes;
};
struct Code
{
 char code[8+1]; //each code is supposed to have 8 as max size. +1 for the null terminator
};
struct GenericNode
{
 void *data;
 struct GenericNode *next;
};
void GenericPush(struct GenericNode **list, void *data)
{
 struct GenericNode *new = malloc(sizeof(struct GenericNode));
 new->data = data;
 new->next = NULL;
 if(*list)
 new->next = *list;
 *list = new;
 return;
}
struct GenericNode * GenericPop(struct GenericNode **list)
{
 struct GenericNode *item = NULL;
 if(*list)
 {
 item = *list;
 *list = item->next;
 }
 return item;
}
void GenericFree(struct GenericNode *item)
{
 if(item && item->data)
 free(item->data);
 free(item);
 return;
}
struct CodeGroup *newGroup(char *name)
{
 struct CodeGroup *new = malloc(sizeof(struct CodeGroup));
 strcpy(new->name, name);
 new->codes = NULL;
 return new;
}
struct Code *newCode(char *code)
{
 struct Code *new = malloc(sizeof(struct Code));
 strcpy(new->code, code);
 return new;
}
int main (int argc, char *argv[]) {
 struct GenericNode *groupList = NULL;
 struct GenericNode *item = NULL;
 struct GenericNode *item2 = NULL;
 struct CodeGroup *codeGroup = NULL;
 struct Code *code = NULL;
 /*
 Data is structured as follows:
 group list
 |
 \--- group
 | \--- name
 | \--- codes list
 | \-- code
 | \-- code
 | ...
 |
 \--- group
 | \--- name
 | \--- codes list
 | \-- code
 | \-- code
 | ...
 |
 ...
 */
 codeGroup = newGroup("group one");
 GenericPush(&codeGroup->codes, (void*)newCode("10000001"));
 GenericPush(&codeGroup->codes, (void*)newCode("10000002"));
 GenericPush(&codeGroup->codes, (void*)newCode("10000003"));
 GenericPush(&codeGroup->codes, (void*)newCode("10000004"));
 GenericPush(&codeGroup->codes, (void*)newCode("10000005"));
 GenericPush(&groupList, (void*)codeGroup);
 codeGroup = newGroup("group two");
 GenericPush(&codeGroup->codes, (void*)newCode("20000001"));
 GenericPush(&codeGroup->codes, (void*)newCode("20000002"));
 GenericPush(&codeGroup->codes, (void*)newCode("20000003"));
 GenericPush(&codeGroup->codes, (void*)newCode("20000004"));
 GenericPush(&codeGroup->codes, (void*)newCode("20000005"));
 GenericPush(&groupList, (void*)codeGroup);
 // prints all groups and its codes, also freeing them in the process:
 while(item = GenericPop(&groupList))
 {
 codeGroup = (struct CodeGroup*)item->data;
 printf("Group name: %s\n", codeGroup->name);
 printf("\'%s\' codes:\n", codeGroup->name);
 while(item2 = GenericPop(&codeGroup->codes))
 {
 code = (struct Code*)item2->data;
 printf("%s\n", code->code);
 GenericFree(item2);
 }
 GenericFree(item);
 }
 return 0;
}

Output:

Group name: group two
'group two' codes:
20000005
20000004
20000003
20000002
20000001
Group name: group one
'group one' codes:
10000005
10000004
10000003
10000002
10000001

Looking for recommendations on how to improve, simplify or standardize this code, or tips on how to make it better overall.

Answer 1

Good formatting

Include.h?

A companion Generic.h with the public functions definitions is a common and good exercise.

Consider a function to test empty-ness

bool GenericEmpty(const struct GenericNode *list);

Consider a function to peek at the top data

void *GenericPeek(const struct GenericNode *list);

Consider a pop that also frees

void *GenericPopData(struct GenericNode **list) {
 void *data = NULL;
 if (list && *list) {
 struct GenericNode *node = *list;
 *list = node->next;
 data = node->data;
 free(node);
 }
 return data;
}

Drop unneeded cast

// GenericPush(&codeGroup->codes, (void*)newCode("10000001"));
GenericPush(&codeGroup->codes, newCode("10000001"));
// code = (struct Code*)item2->data;
code = item2->data;

Allocate per the referenced data

The 2nd form is easier to code right, review and maintain.

// new = malloc(sizeof(struct GenericNode));
new = malloc(sizeof *new);

Robust code checks allocations

p = malloc(...);
if (p == NULL) Handle_Error();

Avoid overruns

// Best way to detect potential overrun is very case dependent.
if (strlen(name) >= sizeof new->name) Handle_error();
else strcpy(new->name, name);

Unneeded test

free(NULL) is OK.

// if(item && item->data)
if(item)
 free(item->data);

Keyword

_Generic is a C keyword. Recommend a different common prefix. Maybe CommonNode?

For me, I tend avoid C++ keywords in C like new, others.

• liewl: Thank you, great tips! The pop that also frees is a great idea, I don't need the GenericFree() function if pop works that way. Regarding the function that checks for emptiness, can't I just check the list pointer? It will contain NULL if the list is empty.
• chux: @liewl "Regarding the function that checks for emptiness, can't I just check the list pointer? It will contain NULL if the list is empty" --> Yes that is reasonable and idiomatic - if the calling code has awareness of that when struct GenericNode * == NULL (and only then) that that means an empty list. To promote data hiding/abstraction, a define or inline GenericEmpty() could hide that NULL test. Perhaps at a later time, code returns a special pointer to indicate the list is invalid due to out-of-memory. Then the test is list == NULL || list == special. Your call.