Some of my points below apply to multiple parts of the program. I'm
only going to mention them once each.
#include <stdio.h>
#include <stdlib.h>
/* stack element */
This comment provides little value. I can guess that a type called
`stack_element_t' is used to represent stack elements. (Particularly
with syntax colouring editors, even `pointless' comments can serve to
break up the monotony of a source file. Even so, I don't think I'd
bother with this one.)
typedef struct stack_element_t {
void* payload;
struct stack_element_t * next;
Your formatting of pointer declarators is inconsistent. I recommend:
void *payload;
struct stack_element_t *next;
Reasons for preferring this form, with the `*' next to the name, have
been discussed recently in other threads.
I'd (mildly) recommend against putting `_t' on the end of type names.
Such names are reserved by POSIX (IEEE 1003.1--2001, 2.2.2) for
implementations; while this isn't strictly a C issue, it does affect
potential portability.
/* stack */
typedef struct stack_t {
stack_element_t * tos;
size_t size;
} stack_t;
/* initialise stack object */
stack_t *
stack_init()
{
stack_t* stack;
stack = malloc(sizeof(stack_t));
I'd write
stack = malloc(sizeof(*stack));
here.
stack->tos = NULL;
stack->size = 0;
You've not checked the return value from malloc. I don't have a problem
with simply terminating the program on malloc failure, but others might
-- but terminating the program and hoping that indirecting through a
null pointer will crash it aren't the same. Of course, dealing with
malloc failure more gracefully means that you'll have to decide on some
way of reporting the error to the caller.
return stack;
}
/* pop value from stack */
void*
stack_pop(stack_t* stack)
{
stack_element_t* element;
if (stack->size == 0)
return NULL;
This is a fine check for stack-emptiness. However, I'd write it as
if (!stack->tos)
return (0);
Why?
* I have a strange habit of putting return values in parentheses.
Once, a very long time ago, the parentheses were required, and this
habit has stuck. I should have stopped doing this when I switched
brace styles, but I didn't and I'm not going through another style
change now.
* I never use NULL because I consider it deceptive. Since NULL
needn't expand to more than an unadorned `0', writing NULL as an
argument to a variadic function which expects a pointer can cause
undefined behaviour; but NULL looks like it ought to be safer than
this. I find that writing `0' instead of `NULL' keeps me alert to
these kinds of problems.
* I've used `stack->tos' rather than `stack->size' to decide whether
the stack is empty. If the stack is going to work at all properly,
then stack->tos will be null when it's empty, and non-null
otherwise. But stack->size is maintained as a secondary indicator
and might -- conceivably -- get out of sync. Given this, it might
be worth putting something like
assert(!stack->tos == !stack->size);
above. (This ties in with another recent thread about such
conditions. Note the use of `!' to canonicalize the two sub-
conditions to 0-or-1 truth values.)
element = stack->tos;
stack->tos = element->next;
void* result = element->payload;
free(element);
stack->size--;
return result;
}
/* destroy stack */
void
stack_destroy(stack_t* stack)
{
while((void*)stack_pop(stack) != NULL);
Why cast the result of stack_pop to void * when it already returns one?
I'd write
while (stack_pop(stack));
though some would like to see the semicolon on the next line:
while (stack_pop(stack))
;
which can make the empty loop body clearer. (I rely on my editor's
automatic indentation to detect such things, and tend to trust the
indentation of code I'm reading -- or beat it up with GNU Indent if it's
obviously awful.)
Notice that I've dropped the `!= NULL' part of the condition. I
consider it redundant; but others may not.
Moving on to a higher-level issue: presumably someone ought to have
freed the individual stack-element payloads by now. You've just leaked
them all. I think I'd actually replace the above loop by
assert(!stack->tos);
Then I can leave freeing of the stack elements to my caller, who
presumably has a better idea of how it should be done, and can detect
when the caller fails to do this.
Oh, better
#include <assert.h>
at the top of the program. I don't believe in setting NDEBUG.
free(stack);
}
/* push element onto stack */
void
stack_push(stack_t* stack, void* value)
{
stack_element_t* entry;
entry = malloc(sizeof(stack_element_t));
Again, you've not checked the return value from malloc. I said I wasn't
going to repeat myself, but this is sufficiently important.
I'd write
stack_element_t *entry = malloc(sizeof(*entry));
and save a line. Well, in truth, I wouldn't:
stack_element_t *entry;
if ((entry = malloc(sizeof(*entry))) != 0)
/* ... do something about it ... */;
(The `!= 0' is an old historical wart which prevents some old compilers
from complaining about the assignment. Usually I'd omit that as
redundant.)
entry->next = stack->tos;
entry->payload = value;
stack->tos = entry;
stack->size++;
}
/* entry point */
int
main()
{
stack_t* stack;
stack = stack_init();
printf("size pre-push: %d\n", stack->size);
stack_push(stack, "entry 1");
stack_push(stack, "entry 2");
stack_push(stack, "entry 3");
printf("size post-push: %d\n", stack->size);
Some may criticize you for exposing the stack length as a structure
member rather than through a function; I don't see the problem myself.
printf("%s\n", (char*)stack_pop(stack));
This cast isn't actually necessary (6.2.5p25). It's probably a good
habit anyway: if it had been a pointer to anything other than character
type, the cast would most definitely be necessary in this context.
printf("%s\n", (char*)stack_pop(stack));
printf("%s\n", (char*)stack_pop(stack));
printf("size post-pop: %d\n", stack->size);
stack_destroy(stack);
return 0;
}
I'd say not bad overall. Please check the return value from malloc.
-- [mdw]
