Re: [patch 3/2] drivers/char/vt.c: remove unnecessary code

Horst von Brand
2005-02-28T14:58:43Z

colbuse@ensisun.imag.fr said:
> Surlignage Russell King <rmk+lkml@arm.linux.org.uk>:
> > On Mon, Feb 28, 2005 at 02:13:57PM +0100, colbuse@ensisun.imag.fr wrote:
> > > NPAR times :-). As I stated, npar is unsigned.

> > I think that's disgusting then - it isn't obvious what's going on, which
> > leads to mistakes.

> > For the sake of a micro-optimisation such as this, it's far more important
> > that the code be readable and easily understandable.
> >
> > Others may disagree.

> I agree :) .

I agree too.

> But, if we look to the code, we can notice that there is
> actually no reason for npar to be unsigned. What do you think of this
> version?

>
> --- old/drivers/char/vt.c 2004-12-24 22:35:25.000000000 +0100
> +++ new/drivers/char/vt.c 2005-02-28 12:53:57.933256631 +0100
> @@ -1655,9 +1655,9 @@
> vc_state = ESnormal;
> return;
> case ESsquare:
> - for(npar = 0 ; npar < NPAR ; npar++)
> + for(npar = NPAR - 1; npar >= 0; npar--)
> par[npar] = 0;
> + npar++;

Completely unreadable. What do you think you might gain this way? I doesn't
matter in which order the par[i] are set to zero, does it?

Please stick to plain, down-to-earth, colloquial C unless there is a
/measurable/ advantage in not doing so, and it matters overall (i.e., the
code in question runs many times a second). Readability first, traded in
for efficiency only when it /really/ matters. Given today's gcc, you'll
probably just confuse it into generating bad code by using cute tricks, as
it has been tuned to the typical language usage. And gcc is a moving
target, what might be a good idea today could be the worst possible thing
to do in a year or so (or even today with a different architecture).
--
Dr. Horst H. von Brand User #22616 counter.li.org
Departamento de Informatica Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria +56 32 654239
Casilla 110-V, Valparaiso, Chile Fax: +56 32 797513