[cfe-dev] Side effect of conditional operator

[Chris Lattner]

On Nov 13, 2007, at 1:12 PM, Sanghyeon Seo wrote:

> 2007/11/14, Chris Lattner <clattner at apple.com>:
>> ?: itself doesn't have side effects.  The warning is reasonable in
>> this code:
>>
>>    i < j ? (sum += i) : (sum += j);
>>
>> The issue is that the ?: expression itself returns a value, and that
>> value is being ignored.  Why not use an if statement in this case?
>
> i = 1;
>
> The issue is that = expression itself returns a value, and that value
> is being ignored...?

Right, but assignment has a side effect.  Imagine if they wrote "i ==  
1".  We do emit a warning for it, because there is computation being  
done that is ignored.

> In this case, ?: expression as a whole clearly
> has a local side effect.

Right, but the result that is computed doesn't :).

> The affected code is from Expat 2.0.1, xmlparse.c, lookup function,
> which implements linear probing of open addressing hash table.
>
> unsigned long h = hash(name);
> unsigned long mask = (unsigned long)table->size - 1;
> unsigned char step = 0;
> i = h & mask;
> while (table->v[i]) {
>   if (keyeq(name, table->v[i]->name))
>     return table->v[i];
>   if (!step)
>     step = PROBE_STEP(h, mask, table->power);
>   i < step ? (i += table->size - step) : (i -= step);
> }
>
> Yes, it can be written as i = (i - step) % table->size; (I think).

Why not just:
   if (i < step)
     i += table->size - step;
   else
     i -= step;

or:
   i += i < step ? (table->size - step) : -step;
etc.

There are lots of ways to work around the warning.  I tend to think  
that either of the two above are more obvious and clear than the code  
they wrote. :)

> Whether this diserves a warning is a matter of opinion.

Yep.  Warnings in general are very contentious.  The basic issue is  
that warnings are designed to be emitted on *valid code* that is  
questionable in some way.  The questionability of the code varies  
based on who wrote it :)

-Chris