HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Generic stack in C

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
genericstackstackoverflow

Problem

I tried to implement a generic stack in C using void pointers and tried to keep it as simple as possible by delegating all responsibility to the caller of the functions and avoiding more sophisticated approaches.

stack.h

#ifndef STACK_H
#define STACK_H

#include 

struct Stack {
    void *data;
    struct Stack *next;
};

/*
 * We declare a pointer to a Stack structure thereby making use of incomplete
 * types. Clients that pull in stack.h will be able to declare variables of type
 * pstack which are pointers to pointers to Stack structures.
 * */
typedef struct Stack *pstack;

bool is_empty(pstack *s);
void make_empty(pstack *s);
void push(struct Stack **s, void *new_num);
void *pop(pstack *s);

#endif /* STACK_H */


stack.c

#include 
#include 
#include "stack.h"

bool is_empty(pstack *s) { return !s; }

void make_empty(pstack *s)
{
    if (!is_empty(s))
        pop(s);
}

void *pop(pstack *s)
{
    struct Stack *tmp;
    void *i;

    if (is_empty(s))
        exit(EXIT_FAILURE);

    tmp = *s;
    i = (*s)->data;
    *s = (*s)->next;
    free(tmp);
    return i;
}

void push(struct Stack **s, void *new_num)
{
    struct Stack *new_node = malloc(sizeof(struct Stack));
    if (!new_node)
        exit(EXIT_FAILURE);

    new_node->data = new_num;
    new_node->next = *s;
    *s = new_node;
}


stackclient.c

#include 
#include 
#include "stack.h"

int main(void)
{
    pstack s1;
    void *n;
    int i = 1;
    int j = 2;

    push(&s1, &i);
    push(&s1, &j);

    n = pop(&s1);
    printf("Popped %d from s1\n", *((int *)n));
    n = pop(&s1);
    printf("Popped %d from s1\n", *((int *)n));

    exit(EXIT_SUCCESS);
}

Solution

I see some things that may help you improve your code.

Use all required #includes

The stack.c file has #include but it appears to me that no functions from it are needed. On the other hand, it uses the bool from stdbool.h but doesn't include it directly. One might argue (correctly!) that because it include stack.h that it's included indirectly, but I'd advocate that it's also in the implementation file so that it's obvious that the bool used there is the stdbool.h version.

Use const where practical

The is_empty function does not alter the stack, so that argument should express that notion by using the const keyword:

bool is_empty(const pstack *s) { return !s; }


Combine typedef and structure declaration

It's a common C idiom to combine the typedef and the stack declaration.

typedef struct Stack {
    void *data;
    struct Stack *next;
} *pstack;


This keeps them together which helps the reader of the code.

Don't misuse exit

The final statement in main currently is:

exit(EXIT_SUCCESS);


However, code to return EXIT_SUCCESS is automatically generated if the code reaches the end of main so this is not needed. Also, it may be more appropriate to return NULL instead of aborting the program with exit if too many pop operations are requested.

Consider object ownership

The stack only stores pointers, but does not "own" the objects to which those pointers point. This means that if you push pointers to stack objects with auto scope, the stack will contain invalid pointers if the underlying items go out of scope. An alternative would be to create a copy of the object and allow the stack to own them.

Don't rely on values not set

The pstack relies on the value being 0, (in is_empty and make_empty) but does not explicity set it to that (r any) value. If your code checks for a specific value, it should also set it.

Code Snippets

bool is_empty(const pstack *s) { return !s; }
typedef struct Stack {
    void *data;
    struct Stack *next;
} *pstack;
exit(EXIT_SUCCESS);

Context

StackExchange Code Review Q#96357, answer score: 6

Revisions (0)

No revisions yet.