It's just me, I think, but return()s in loops can be bad.

I was reviewing some code tonight. It was a simple linked-list match which originally looked like:

obj_t *
lookup(match_t key)
{
        obj_t *p;

        for (p = list_head(); p; p = list_next()) {
                if (p->val == key)
                        return (p);
        }

        return (NULL);
}

Not bad. But it turns out the list in question needed mutually exclusive access, so the reviewee inserted the mutex into this code.

obj_t *
lookup(match_t key)
{
        obj_t *p;

        mutex_enter(list_lock());
        for (p = list_head(); p; p = list_next()) {
                if (p->val == key) {
                        mutex_exit(list_lock());
                        return (p);
                }
        }

        mutex_exit(list_lock());
        return (NULL);
}

Eeesh, two places to call mutex_exit(). I suppose a good compiler would recognize the common basic blocks and optimize them out, but that's still mildly ugly to look at. Still, that above code just rubbed me the wrong way, even though I KNOW there are other bits of Illumos that are like the above. I didn't block the reviewer, but I did write down what I thought it should look like:

obj_t *
lookup(match_t key)
{
        obj_t *p;

        mutex_enter(list_lock());

        p = list_head();
        while ( p != NULL && p->val != key)
                p = list_next();

        mutex_exit(list_lock());
        return (p);
}

That seems simpler. The operation is encapsulated in the mutex_{enter,exit} section, and there are no escape hatches save those in the while boolean. (It's the always-drop-the-mutex-upon-return that makes language constructs like monitors look appealing.)

I think I'm probably making a bigger deal out of this than I should, but the last code looks more readable to me.

One thing the reviewee suggested to me was that a for loop like before, but with breaks, would be equally clean w.r.t. only having one place to drop the mutex. I think the reviewee is right, and it allows for more sophisticated exits from a loop.

Some people would even use "goto fail" here, but we know what can happen when that goes wrong. :)