Crisis of Style
2006-05-31

I was reviewing some code today, which had the following fragment:

struct option {
        char *name;
        char *format;
        struct universe *universe;
        unsigned code;
        int refcnt;
};

struct option *new_option (name, file, line)
        const char *name;
        const char *file;
        int line;
{
        struct option *rval;
        int len;

        len = strlen(name);

        rval = dmalloc(sizeof(struct option) + len + 1, file, line);

        if(rval) {
                memcpy(rval + 1, name, len);
                rval->name = (char *)(rval + 1);
        }

        return rval;
}

So this is a pretty basic constructor. The code is correct. My concern is that it is not as straightforward as it could be, in that it allocates a single block of memory and then points the variable size data, name, after the fixed size data. If I were going to write it, I would have allocated two blocks of memory:

struct option *new_option (name, file, line)
        const char *name;
        const char *file;
        int line;
{
        struct option *rval;
        int len;

        rval = dmalloc(sizeof(struct option), file, line);
	if (rval == NULL) {
		return NULL;
	}

        len = strlen(name);
	rval->name = dmalloc(len+1, file, line);
	if (rval->name == NULL) {
		dfree(rval);
		return NULL;
	}

        memcpy(rval->name, name, len);	/* NUL-terminated by dmalloc() */

        return rval;
}

This involves a bit more code, and two memory allocations instead of one. So there is a performance penalty, although it is so small I think you'd have a hard time measuring it in practice. However, I think it's less "tricky", and therefore easier to read, meaning:

• Easier to understand.
• Less likely to be modified incorrectly.

Is it actually easier to understand with two allocations, or am I crazy?

Shane Kerr
shane@time-travellers.org