Suggestion for a new warning

["J. Kean Johnston" <jkj at sco dot com>]

Hello again.

Recently I have had cause to argue the following code with several
people:

char array[256],*p;
p = some_arbitrary_string;
while (*p)
  *p++ = array[*p];  /* THIS IS THE LINE THAT IS BAD */

Syntactically, this is correct. If you know your precedence rules well
enough it may even be obvious. But it is still (IMHO) very bad code, and
most people seem to agree.  I have had this out with a few of the compiler
experts here, as well as two professors at UCSC, and the general concensus
is this (I qm quoting liberally here):

> ++ has a higher precedence than =. ANSI C, the K&R books and the grammars
> all agree on this. No problem here.

So far so good :-)

> What is not defined is whether or not the RH operand of = is
> evaluated before or after *p++ is evaluated.  Which means that
> the subscript [*p] could be the old or the new value of p.
Now, if this statement is true (and many seem to think it is), then
what I suggest is either a warning (but preferably an error, or at
least a warning that is ALWAYS on) to the effect that the expression
evaluation is order dependant. Some suitable text like:

warning: left/right evaluation order in expression is dubious.

> Moral: when a variable involved in side effects
> like ++ or -- is used in an expression, don't use it more than
> once.  Only the operators &&, ||, ?:, and , specifically define
> the order of execution of their operands.

This gives a clue to an even better warning candidate:
warning: operator with side-effects causes dubious evaluation.

It seems that the expression is evaulated differently depending on
whether or not the compiler evaluates the LHS or the RHS first. Yes,
the associativity is from right to left for the = operator, but the
expression is still expremely suspect, and I do not think this is an
associativity issue.  Generating a warning message should ring a loud
bell to users (and this can be documented easily) and encourage them
to write the expression in the slightly longer (but much clearer and
less ambiguous) way:

   *p = array[*p];
   p++;

For openers, this makes debugging much easier, as you can watch the
expression changing (if you are using a source level debugger). Secondly,
it makes the code visibly clearer what it is doing.  Having the warning
being emitted would encourage (again, in my opinion) better programming
practices.  I was taught once by a programmer I respect to "always code
for your debugger. If its not easy to step through with your debugger
its not maintainable."  Wise words.

JKJ