C pointer based growable stack rev2

Question

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:

1. 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...
2. How should a failed malloc()/realloc() be handled? Return an error code? Exit? Return NULL?

Again, I would greatly appreciate any suggestions/criticism:

#include <stdlib.h>
#include <stdint.h>
#include <string.h>
#include <stdbool.h>
#include <stdio.h>
#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->count = 0;
 stack->capacity = initialCapacity;
 stack->itemSize = itemSize;
 stack->data = (uint8_t *)malloc(size);
 if (stack->data == NULL)
 {
 // TODO
 }
 memset(stack->data, 0, size);
}
bool stack_is_empty(const Stack *stack)
{
 return stack->count == 0;
}
void stack_push(Stack *stack, void *item)
{
 if (stack->count >= stack->capacity)
 {
 stack->capacity *= 2;
 stack->data = (uint8_t *)realloc(stack->data, stack->capacity * stack->itemSize);
 if (stack->data == NULL)
 {
 // TODO
 }
 }
 unsigned offset = stack->count * stack->itemSize;
 memcpy(stack->data + offset, item, stack->itemSize);
 stack->count++;
}
void* stack_pop(Stack *stack)
{
 if (stack_is_empty(stack) == true)
 {
 // TODO
 }
 uint8_t *item = (uint8_t *)malloc(stack->itemSize);
 if (item == NULL)
 {
 // TODO
 }
 stack->count--;
 unsigned offset = stack->count * stack->itemSize;
 memcpy(item, stack->data + offset, stack->itemSize);
 return (void *)item;
}
void stack_destroy(Stack *stack)
{
 free(stack->data);
 stack->count = 0;
}
int main(void)
{
 Stack stack;
 stack_init(&stack, sizeof(int), 0);
 for (int i = 0; i < MAX_ITEMS; i++)
 {
 stack_push(&stack, (void *)&i);
 }
 while (!stack_is_empty(&stack))
 {
 int value;
 value = *((int *)stack_pop(&stack));
 printf("%d\n", value);
 }
 stack_destroy(&stack);
}

Answer 1

 stack->data = (uint8_t *)realloc(stack->data, stack->capacity * stack->itemSize);

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

void *newData = realloc(stack->data, stack->capacity * stack->itemSize);
if (!newData) {
 // Insert error handling - you'll want to restore the old stack->capacity here
 // or (better) wait until we're okay before updating.
 return FAILURE:
}
stack->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(&stack, &foo);
// no need to check for null, or to call free()