Wednesday, February 26, 2014

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. :)

3 comments:

  1. I've learned that what people consider "obvious" code varies from person to
    person. I'm talking about various idioms because that's really what this is
    about. I agree with your statement that returns can be bad. Given your
    example, I'd probably go the for-loop & break route. Keep in mind that I'm
    still used to a Linux kernel-style linked lists which you iterate this way:

    list_for_each(cur, list) {
    /* cur is the current node */
    }

    This makes the "shove the key comparison into the loop" approach impossible,
    so I don't even *think* of that approach even when given a slightly
    different API. That is, when I *write* the code. Once the characters are
    on the screen, I may reshuffle them to make the result "cleaner" and
    "better".

    Locking is very important. I use empty lines to make it easier to spot
    locking bugs. If the protected region is simple (one line, or a one line
    loop) I'm likely to do:

    total = 0;

    mutex_enter(...);
    for (i = 0; i < n; i++)
    total += arr[i];
    mutex_exit(...);

    return total;

    otherwise:

    mutex_enter(...);

    /* complex code here */

    mutex_exit(...);

    return ...;

    I find the visually separated mutex operations draw my eye to them and so
    I'm more likely to remember that I can't jump (continue, break, goto,
    return) too far.

    If you haven't already, I'd suggest you skim over the Linux kernel coding
    style. There are some useful bits of information that help with code
    readability.

    http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle

    I suppose this is an extremely long-winded way of saying that I agree with
    you that returns may be bad. :)

    Oh, the idea of idioms is somewhat related to people writing C++ like it is
    C. They are different languages and they require a different mindset.

    ReplyDelete
  2. Where are the curly braces in your while loop?

    I think it was drilled into me at Arizona that we should only have one return in a method to make life easier for the optimizer, but it's good practice beyond that.

    Somewhere else, I picked up the habit of always having braces for one line code-blocks with if's and loops. (even if I end up putting all of the code on the same line.

    ReplyDelete
  3. Unknown -> The Illumos style guide, inherited from OpenSolaris/Solaris, isn't on the illumos.org site for some reason. /opt/onbld/bin/cstyle -pP will evaluate your source, however. (And reviewers may be pickier.)

    John --> Single-line boolean and single-line action means no braces. Otherwise you need them.

    ReplyDelete