Side effect of conditional operator

Clang Frontend
#1

Conditional operator expression may have side effect if one of its
operands does.

A patch with a test is attached.

patch-conditional-side-effect (1.08 KB)

#2

This is a very nice patch, but I don't think it is appropriate.

?: 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?

-Chris

#3

i = 1;

The issue is that = expression itself returns a value, and that value
is being ignored...? In this case, ?: expression as a whole clearly
has a local side effect.

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).
Whether this diserves a warning is a matter of opinion.

#4

?: 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. :slight_smile:

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 :slight_smile:

-Chris