Suppose that I need to malloc three times and I would like to make an early return if one of the malloc calls have failed.
My first try:
void test(void) {
int *a, *b, *c;
a = malloc(sizeof(*a));
if (a == NULL)
return;
b = malloc(sizeof(*b));
if (b == NULL) {
free(a); /* since `a` has been `malloc`ed. */
return;
}
c = malloc(sizeof(*c));
if (c == NULL) {
free(a); // since `a` and `b` have
free(b); // been `malloc`ed.
return;
}
}
However, somehow I don't think this is a nice way to handle malloc failures.
My second thought:
a = malloc(sizeof(*a));
b = malloc(sizeof(*b));
c = malloc(sizeof(*c));
if (a == NULL || b == NULL || c == NULL) {
free(a); // I've read that it's fine
free(b); // to `free` a NULL pointer.
free(c);
return;
}
I think that this is better in that it looks clean, but that's because I'm handling only three pointers. If I had more pointers, it wouldn't look clean.
Meanwhile, I've seen a brilliant method which casts pointers into uintptr_t. However, as far as I understand, this technique requires for NULL to be 0.
My last resort:
void *container;
container = malloc(sizeof(int) * 3);
if (container == NULL)
return;
a = getaddr(0);
b = getaddr(1);
c = getaddr(2);
// def: getaddr
static inline int *getaddr(void *base, int index) {
return base + (sizeof(int) * index);
}
By mallocing the total memory I needed, I was able to write the NULL test only once, but it's still a headache to me to do so.
So I wonder whether there are nicer ways or best practices to handle this situation. It would be greatly appreciated if you could enlighten me.
EDIT: First of all, I thank all of you for helping me out. Please let me elaborate the intention of this question.
struct tree stores three pointers which are dynamically allocated:typedef struct tree tree_t;
struct tree {
tree_t *parent; // a pointer to the parent tree.
list_t *children; // a pointer to the list which has the pointers to the child trees of this tree.
// list_t is a linked list I've made.
void *data; // a pointer to the data this tree has.
size_t data_size; // the size of the data.
};
// This function creates a new tree object.
// This function returns a pointer to the newly created tree.
// If failed, returns NULL.
extern tree_t *tree_plant(void *data, size_t data_size) {
// under these conditions, fails to create one.
if (data == NULL || data_size == 0)
return NULL;
tree_t *sapling;
void *tree_data;
list_t *tree_children;
// This is the very point which confuses me.
// I would like to return NULL if failed to malloc.
sapling = malloc(sizeof(tree_t));
if (sapling == NULL)
return NULL;
tree_data = malloc(data_size);
if (tree_data == NULL) {
free(sapling);
return NULL;
}
tree_children = list_create(); // list_create makes a new list.
if (tree_children == NULL) { // list_create returns NULL if it fails to make one, like tree_plant does.
free(sapling);
free(tree_data);
return NULL;
}
// Initializes all fields
sapling->parent = NULL;
sapling->children = tree_children;
sapling->data = tree_data;
sapling->data_size = data_size;
// I've heard it's best practice to copy data when implementing an abstract data type.
memcpy(sapling->data, data, data_size);
// success to make a tree
return sapling;
}
// This is the list implementation.
typedef struct node {
struct node *prev;
struct node *next;
void *data;
size_t data_size;
} node_t;
typedef struct list list_t;
struct list {
node_t *head;
node_t *tail;
size_t size; // the length of this list.
};
extern list_t *list_create(void) {
list_t *list;
list = malloc(sizeof(list_t));
if (list == NULL)
return NULL;
list->head = NULL;
list->tail = NULL;
list->size = 0;
return list;
}
malloc fails is an early return, and the reason I want the early return is, I'm afraid, I don't know. Perhaps because I want a kind of consistency(?) with functions returning NULL when it fails?EDIT: I deeply appreciate all the answers and comments. Those were greatly helpful and interesting to me.
You can pass NULL to free, so no checks are necessary.
tree_t *tree_plant( void *data, size_t data_size ) {
// ...
tree_t *sapling = NULL;
void *tree_data = NULL;
sapling = malloc( sizeof( tree_t ) );
if ( !sapling )
goto ERROR;
tree_data = malloc( data_size );
if ( !tree_data )
goto ERROR;
list_t *tree_children = list_create();
if ( !tree_children )
goto ERROR;
// ...
return sapling;
ERROR:
free( tree_data );
free( sapling );
return NULL;
}
The above is specialization of the following general approach which I find clearer:
tree_t *tree_plant( void *data, size_t data_size ) {
// ...
tree_t *sapling = malloc( sizeof( tree_t ) );
if ( !sapling )
goto ERROR1;
void *tree_data = malloc( data_size );
if ( !tree_data )
goto ERROR2;
list_t *tree_children = list_create();
if ( !tree_children )
goto ERROR3;
// ...
return sapling;
ERROR3:
free( tree_data );
ERROR2:
free( sapling );
ERROR1:
return NULL;
}
The very obvious construction-destruction symmetry with minimal code and minimal indenting is what makes this approach very clean.
Furthermore, this approach is incredibly flexible without sacrificing any readability.
bool test( void ) {
bool rv = false;
FILE *f1 = fopen( ... );
if ( !f1 ) {
// Output relevant error message here.
goto ERROR1;
}
FILE *f2 = fopen( ... );
if ( !f2 ) {
// Output relevant error message here.
goto ERROR2;
}
// Do something with file handles here.
rv = true;
fclose( f2 );
ERROR2:
fclose( f1 );
ERROR1:
return rv;
}