Here are some things that may help you improve your code.
Use NULL
instead of 0
for pointers
The value 0
is an integer, but the value NULL
is an implementation-defined null-pointer constant. It is not guaranteed to have the value 0.
Use include guards
There should be an include guard in the .h
file. That is, start the file with:
#ifndef LIST_H
#define LIST_H
// file contents go here
#endif // LIST_H
Simplify your code
I'm not sure why the x_upcast
and data_upcast
code exists. Maybe the intent was to more cleanly separate the data
type (an int
here) from the rest of the code. However, consider that it could instead be written like this:
static struct Link *x_upcast(struct X *const x) {
return (struct Link *)x;
}
Better though, in my opinion, would be to eliminate it entirely. The single place it's used is in ListForEach
:
for(x = list->head.next; (next_x = x->next); x = next_x)
action(&(x_upcast)(x)->data);
This could be more clearly written as:
for(x = list->head.next; (next_x = x->next); x = next_x)
action(&((struct Link *)x)->data);
This also brings us to the next suggestion.
Use the appropriate data type
The code, as posted, appears to treat pointers to the data value and pointers to a Link
identically. This is a problem because it misleads the reader. For example, the get_link
code creates and partially initializes a Link
but claims to be returning an int *
. This would be much more clear if, instead, the code were to actually return a struct Link *
. In other words, the interface should guide correct usage rather than encourage incorrect usage. As an example, this code compiles just fine:
int n = 99;
ListPush(&list, &n);
However this is a runtime disaster waiting to happen. We would probably prefer that it not even compile because what ListPush
actually requires is a pointer to the data member of an already created struct Link
. The next suggestion addresses this problem.
Rethink your interface
If LinkPush
really requires a List
and a Link
, let's declare it that way. Instead of this:
void ListPush(struct List *const list, int *const add);
use this:
void ListPush(struct List *const list, struct Link *const newnode);
Now compiler will actually assist and point out bad usage like the code mentioned previously. This eliminates the type name X
and also requires some redefinitions of other things such as List
and Action
which now look like this:
struct List { struct Link head, tail; };
typedef void (*Action)(struct Link *const);
Use better names
The type name List
is good, but the type name X
is not. The first name explains something about what the variable means within the context of the code, but the latter is opaque and non-descriptive.
Better describe the responsibilities of the data structure
It is quite important to note that this implementation of a linked list assumes that some other entity is creating (and presumably deleting) its nodes. It gives the user some flexibility, as it would allow the use of statically or dynamically allocated memory, but it's worth explicitly mentioning to the user of the code in a comment.