Using if (!!(expr)) instead of if (expr)

While reading the example code provided by Texas Instruments for their SensorTag I came across the following snippet.

void SensorTagIO_processCharChangeEvt(uint8_t paramID) { 
    ...

    if (!!(ioValue & IO_DATA_LED1)) {
        PIN_setOutputValue(hGpioPin, Board_LED1, Board_LED_ON);
    } else {
        PIN_setOutputValue(hGpioPin, Board_LED1, Board_LED_OFF);
    }

    if (!!(ioValue & IO_DATA_LED2)) {
        PIN_setOutputValue(hGpioPin, Board_LED2, Board_LED_ON);
    } else {
        PIN_setOutputValue(hGpioPin, Board_LED2, Board_LED_OFF);
    }

    if (!!((ioValue & IO_DATA_BUZZER))) {
        Clock_start(buzzClockHandle);
    }
    ...
}

The declaration is like this (in the same file).

#define IO_DATA_LED1   0x01
static uint8_t ioValue;

Does if (!!(ioValue & IO_DATA_LED1)) offer any advantage over if (ioValue & IO_DATA_LED1)?

Applying the logical not (!) operator twice has the purpose of normalising a value to be either 0 or 1. In a control expression of an if-statement, that doesn't make any difference. The if-statement only cares about the value being zero or nonzero, the little !! dance is completely useless.

Some coding style guides might mandate this kind of dance, which could be the reason why the TI code you posted does it. I haven't seen any that do so though.

The expression !!x, or !(!x), means 1 if x is a true value (a nonzero number or a nonnull pointer), otherwise 0. It is equivalent to x != 0, and it's nearly the same as C99 (_Bool)x but available in compilers that predate C99 or whose developers have chosen not to implement C99 (such as cc65 which targets the MOS 6502).

The conditional as a whole is equivalent to the following:

if (ioValue & IO_DATA_LED1) {
    /* what to do if the IO_DATA_LED1 bit is true */
} else {
    /* what to do if the IO_DATA_LED1 bit is false */
}

In C, it means "if the bitwise AND of those two values is nonzero, execute the block."

But some coding style guides might prohibit a bitwise AND (&) at the top level of an if statement's condition, assuming it to be a typo for the logical AND (&&). It's in the same class of mistakes as using = (assignment) instead of == (equality comparison), for which many compilers offer a diagnostic. GCC Warning Options describes diagnostics like these:

-Wlogical-op: Warn about suspicious uses of logical operators in expressions. This includes using logical operators in contexts where a bit-wise operator is likely to be expected.

-Wparentheses: Warn if parentheses are omitted in certain contexts, such as when there is an assignment in a context where a truth value is expected

Use of a paraphrase such as (a & B) != 0, (_Bool)(a & B), or !!(a & B) communicates to the compiler and to other developers that use of a bitwise operator was intentional.

See also a related answer about !!x in JavaScript.

In MSVC converting an integer to a bool implicitly in an if statement can generate a warning. Doing so via !! does not. Similar warning may exist in other compilers.

So supposing the code was compiled with that warning enabled, and a decision to treat all warning as errors, using !! is a short and portable way to say "yes, I want this integer to be a bool".

Although silencing the compiler warning for the bit-wise & is the most likely, this looks like it could also be the result of a refactoring to add enums for readability from:

PIN_setOutputValue(int,int,bool); //function definition
PIN_setOutputValue(hGpioPin, Board_LED1,!!(ioValue & IO_DATA_LED1));
PIN_setOutputValue(hGpioPin, Board_LED2,!!(ioValue & IO_DATA_LED2));
//note: the !! is necessary here in case sizeof ioValue > sizeof bool
//otherwise it may only catch the 1st 8 LED statuses as @M.M points out

to:

enum led_enum {
  Board_LED_OFF = false,
  Board_LED_ON = true
};
PIN_setOutputValue(int,int,bool); //function definition
//...
PIN_setOutputValue(hGpioPin, Board_LED1,!!(ioValue & IO_DATA_LED1)?Board_LED_ON:Board_LED_OFF);
PIN_setOutputValue(hGpioPin, Board_LED2,!!(ioValue & IO_DATA_LED2)?Board_LED_ON:Board_LED_OFF);

Since that exceeded the 80 character limit it was then refactored to

if (!!(ioValue & IO_DATA_LED1)) {
    PIN_setOutputValue(hGpioPin, Board_LED1, Board_LED_ON);
} else {
    PIN_setOutputValue(hGpioPin, Board_LED1, Board_LED_OFF);
}

if (!!(ioValue & IO_DATA_LED2)) {
    PIN_setOutputValue(hGpioPin, Board_LED2, Board_LED_ON);
} else {
    PIN_setOutputValue(hGpioPin, Board_LED2, Board_LED_OFF);
}

Personally I would have preferred the initial version for readability, but this version is common when lines of code are used as a metric (I'm surprised it didn't declare variables for each state, set each state separately and then use that).

The next version of this "Best Practice" code might look like:

bool boardled1State;
bool boardled2State;
//...

boardled1State = !!(ioValue & IO_DATA_LED1);
boardled2State = !!(ioValue & IO_DATA_LED2);
//...

if (boardled1State) {
    PIN_setOutputValue(hGpioPin, Board_LED1, Board_LED_ON);
} else {
    PIN_setOutputValue(hGpioPin, Board_LED1, Board_LED_OFF);
}

if (boardled2State) {
    PIN_setOutputValue(hGpioPin, Board_LED2, Board_LED_ON);
} else {
    PIN_setOutputValue(hGpioPin, Board_LED2, Board_LED_OFF);
}
//... and so on

All of that could have been done like this:

for (int i=0;i<numleds;i++)
        PIN_setOutputValue(hGpioPin, i ,!!(ioValue & (1<<i)));

Using if (!!(expr)) instead of if (expr)

Related

Recent Posts