CodeSOD: A Truly Bad Comparison

For C programmers of a certain age (antique), booleans represent a frustrating challenge. But with the addition of stdbool.h, we exited the world of needing to work hard to interact with boolean values. While some gotchas are still in there, your boolean code has the opportunity to be simple.

Mark's predecessor saw how simple it made things, and decided that wouldn't do. So that person went and wrote their own special way of comparing boolean values. It starts with an enum:

typedef enum exop_t {
    EXOP_NONE,
    EXOP_AND,
    EXOP_OR,
    EXOP_EQUAL,
    EXOP_NOTEQUAL,
    EXOP_LT,
    EXOP_GT,
    EXOP_LEQUAL,
    EXOP_GEQUAL,
    EXOP_ADD,
    EXOP_SUBTRACT,
    EXOP_MULTIPLY,
    EXOP_DIV,
    EXOP_MOD,
    EXOP_NEGATE,
    EXOP_UNION,
    EXOP_FILTER1,
    EXOP_FILTER2
};

Yes, they did write an enum to compare booleans. They also wrote not one, but two functions. Let's start with the almost sane one.

static bool compare_booleans (bool bool1,
                              bool bool2,
                              exop_t  exop)
{

    int32_t  cmpresult;

    if ((bool1 && bool2) || (!bool1 && !bool2)) {
        cmpresult = 0;
    } else if (bool1) {
        cmpresult = 1;
    } else {
        cmpresult = -1;
    }

    return convert_compare_result(cmpresult, exop);

}

This function takes two boolean values, and a comparison we wish to perform. Then, we test if they're equal, though the way we do that is by and-ing them together, then or-ing that with the and of their negations. If they're equal, cmpresult is set to zero. If they're not equal, and the first boolean is true, we set cmpresult to one, and finally to negative one.

Thus, we're just invented strcmp for booleans.

But then we call another function, which is super helpful, because it turns that integer into a more normal boolean value.

static boolean
    convert_compare_result (int32_t cmpresult,
                            exop_t exop)
{
    switch (exop) {
    case EXOP_EQUAL:
        return (cmpresult) ? FALSE : TRUE;
    case EXOP_NOTEQUAL:
        return (cmpresult) ? TRUE : FALSE;
    case EXOP_LT:
        return (cmpresult < 0) ? TRUE : FALSE;
    case EXOP_GT:
        return (cmpresult > 0) ? TRUE : FALSE;
    case EXOP_LEQUAL:
        return (cmpresult <= 0) ? TRUE : FALSE;
    case EXOP_GEQUAL:
        return (cmpresult >= 0) ? TRUE : FALSE;
    default:
        printf( "ERR_INTERNAL_VAL\n" );
        return TRUE;
    }
} 

We switch based on the requested operation, and each case is its own little ternary. For equality comparisons, it requires a little bit of backwards logic- if cmpresult is non-zero (thus true), we need to return FALSE. Also note how our expression enum has many more options than convert_compare_result supports, making it very easy to call it wrong- and worse, it returns TRUE if you call it wrong.

At least they made booleans hard again. Who doesn't want to be confused about how to correctly check if two boolean values are the same?

It's worth noting that, for all this code, the rest of the code base never used anything but EXOP_EQUAL and EXOP_NOTEQUAL, because why would you do anything else on booleans? Every instance of compare_booleans could have been replaced with a much clearer == or != operator. Though what should really have been replaced was whoever wrote this code, preferably before they wrote it.

[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!

This post originally appeared on The Daily WTF.

Leave a Reply

Your email address will not be published. Required fields are marked *