C pointer based growable stack rev2 Following on from my previous question: C pointer based growable stack

I have made some improvements (hopefully!) based on the very helpful comments and suggestions.
I have checked that the stack can push and pop doubles now, so hopefully it will also be OK with structs.

I have a could of questions regarding the changes I have made:


I noticed memcpy() didn't work with void * so I switched to using uin8_t; is that recommended/necessary or could I have cast to uint8_t for memcpy()? Not sure which approach is the best...
How should a failed malloc()/realloc() be handled? Return an error code? Exit? Return NULL?


Again, I would greatly appreciate any suggestions/criticism:

#include &lt;stdlib.h&gt;
#include &lt;stdint.h&gt;
#include &lt;string.h&gt;
#include &lt;stdbool.h&gt;
#include &lt;stdio.h&gt;

#define MAX_ITEMS 16

typedef struct {
    uint8_t *data;
    size_t itemSize;
    unsigned count;
    unsigned capacity;
} Stack;

void stack_init(Stack *stack, size_t itemSize, unsigned capacity);
bool stack_is_empty(const Stack *stack);
void stack_push(Stack *stack, void *item);
void* stack_pop(Stack *stack);
void stack_destroy(Stack *stack);

void stack_init(Stack *stack, size_t itemSize, unsigned capacity)
{
    unsigned initialCapacity = capacity == 0 ? 1 : capacity;
    size_t size = initialCapacity * itemSize;

    stack-&gt;count = 0;
    stack-&gt;capacity = initialCapacity;
    stack-&gt;itemSize = itemSize;
    stack-&gt;data = (uint8_t *)malloc(size);

    if (stack-&gt;data == NULL)
    {
        // TODO
    }

    memset(stack-&gt;data, 0, size);
}

bool stack_is_empty(const Stack *stack)
{
    return stack-&gt;count == 0;
}

void stack_push(Stack *stack, void *item)
{
    if (stack-&gt;count &gt;= stack-&gt;capacity)
    {
        stack-&gt;capacity *= 2;
        stack-&gt;data = (uint8_t *)realloc(stack-&gt;data, stack-&gt;capacity * stack-&gt;itemSize);

        if (stack-&gt;data == NULL)
        {
            // TODO
        }
    }

    unsigned offset = stack-&gt;count * stack-&gt;itemSize;
    memcpy(stack-&gt;data + offset, item, stack-&gt;itemSize);
    stack-&gt;count++;
}

void* stack_pop(Stack *stack)
{
    if (stack_is_empty(stack) == true)
    {
        // TODO
    }

    uint8_t *item = (uint8_t *)malloc(stack-&gt;itemSize);

    if (item == NULL)
    {
        // TODO
    }

    stack-&gt;count--;
    unsigned offset = stack-&gt;count * stack-&gt;itemSize;
    memcpy(item, stack-&gt;data + offset, stack-&gt;itemSize);

    return (void *)item;
}

void stack_destroy(Stack *stack)
{
    free(stack-&gt;data);
    stack-&gt;count = 0;
}

int main(void)
{
    Stack stack;
    stack_init(&amp;stack, sizeof(int), 0);

    for (int i = 0; i &lt; MAX_ITEMS; i++)
    {
        stack_push(&amp;stack, (void *)&amp;i);
    }

    while (!stack_is_empty(&amp;stack))
    {
        int value;
        value = *((int *)stack_pop(&amp;stack));
        printf("%d\n", value);
    }

    stack_destroy(&amp;stack);
}
stack-&gt;data = (uint8_t *)realloc(stack-&gt;data, stack-&gt;capacity * stack-&gt;itemSize);



This is wrong, because if realloc() failed, then there's now no way to access the memory previously pointed to by stack-&gt;data.  The correct pattern for realloc() is:

void *newData = realloc(stack-&gt;data, stack-&gt;capacity * stack-&gt;itemSize);
if (!newData) {
    // Insert error handling - you'll want to restore the old stack-&gt;capacity here
    // or (better) wait until we're okay before updating.
    return FAILURE:
}
stack-&gt;data = newData;




I recommend against casting the result of malloc() family when assigning - these functions return void*, which is assignable to any pointer type without a cast.  The unnecessary cast just distracts the reader (because casts generally indicate danger areas in code).  The same is true for conversions from pointer types to void*, such as this one:


return (void *)item;



That can simply be

return item;




The stack_pop interface is difficult to use - callers have to deal with possible null pointer return, and must remember to free() the result.  Instead, consider having the caller provide memory to write into:

void stack_pop(Stack *stack, void *target);


The caller should already know the size required for target, and can now get  results in local (auto) storage:

int foo;
stack_pop(&amp;stack, &amp;foo);
// no need to check for null, or to call free()