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

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

[colbuse]

>> - for(npar = 0 ; npar < NPAR ; npar++)
>> + for(npar = NPAR - 1; npar >= 0; npar--)
>> par[npar] = 0;

>if you really want to clean this up.. 

Well, actually, I was not myself entirely convinced about it... This is the
reason for I wrote "please _don't_ apply this, but tell me what you think about
it.".

>why not use memset() instead ?

Because I simply didn't thought to it :-) .

Hey, that makes fully sense! So far I know, memset() is quicker than 
(or as quick as) a loop, and it remains fully readable (in my opinion :).

Well, such a patch would be : 

--- drivers/char/vt.c   2004-12-24 22:35:25.000000000 +0100
+++ drivers/char/vt2.c  2005-02-28 15:55:11.782717810 +0100
@@ -1655,8 +1655,8 @@
                        vc_state = ESnormal;
                return;
        case ESsquare:
-               for(npar = 0 ; npar < NPAR ; npar++)
-                       par[npar] = 0;
+               /* Setting par[]'s elems at 0.  */
+               memset(par, 0, NPAR*sizeof(unsigned int));
                npar = 0;
                vc_state = ESgetpars;
                if (c == '[') { /* Function key */


Thank you for the suggestion.
What do you think of this one?

(Please note that I'm not trying to get a patch for it _by force_ into the
kernel. If it's a bad idea, let's let thing like they currently are, 
the current loop just works.)

cu

--
Emmanuel Colbus
Club GNU/Linux
ENSIMAG - Departement telecoms

-------------------------------------------------
envoyé via Webmail/IMAG !

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

[Stelian Pop]

On Mon, Feb 28, 2005 at 04:06:14PM +0100, colbuse@ensisun.imag.fr wrote:

> +               /* Setting par[]'s elems at 0.  */
> +               memset(par, 0, NPAR*sizeof(unsigned int));

No need for the comment here, everybody understands C.

Stelian.
-- 
Stelian Pop <stelian@popies.net>    

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

[emmanuel.colbus@ensimag.imag.fr]

On Mon, 28 Feb 2005, Stelian Pop wrote:

> On Mon, Feb 28, 2005 at 04:06:14PM +0100, colbuse@ensisun.imag.fr wrote:
>
> > +               /* Setting par[]'s elems at 0.  */
> > +               memset(par, 0, NPAR*sizeof(unsigned int));
>
> No need for the comment here, everybody understands C.


I knew this code would be understood, but I like comments :-) .

Well, without it, it gives :



--- old/drivers/char/vt.c 2004-12-24 22:35:25.000000000 +0100
+++ new/drivers/char/vt.c 2005-02-28 18:19:11.782717810 +0100
@@ -1655,8 +1655,8 @@
vc_state = ESnormal;
return;
case ESsquare:
- for(npar = 0 ; npar < NPAR ; npar++)
- par[npar] = 0;
+ memset(par, 0, NPAR*sizeof(unsigned int));
npar = 0;
vc_state = ESgetpars;
if (c == '[') { /* Function key */





Any other comments/remarks, or is _this_ patch version acceptable?


--
Emmanuel Colbus
Club GNU/Linux
ENSIMAG - Departement telecoms

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

[Stelian Pop]

On Mon, Feb 28, 2005 at 06:29:39PM +0100, emmanuel.colbus@ensimag.imag.fr wrote:

> Well, without it, it gives :
> 
> 
> 
> --- old/drivers/char/vt.c 2004-12-24 22:35:25.000000000 +0100
> +++ new/drivers/char/vt.c 2005-02-28 18:19:11.782717810 +0100
> @@ -1655,8 +1655,8 @@
> vc_state = ESnormal;
> return;
> case ESsquare:
> - for(npar = 0 ; npar < NPAR ; npar++)
> - par[npar] = 0;
> + memset(par, 0, NPAR*sizeof(unsigned int));
> npar = 0;
> vc_state = ESgetpars;
> if (c == '[') { /* Function key */
> 
> 
> 
> 
> Any other comments/remarks, or is _this_ patch version acceptable?

A not whitespace-mangled version of it could be.

Stelian.
-- 
Stelian Pop <stelian@popies.net>    

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

[Adrian Bunk]

On Mon, Feb 28, 2005 at 06:29:39PM +0100, emmanuel.colbus@ensimag.imag.fr wrote:
> 
> 
> On Mon, 28 Feb 2005, Stelian Pop wrote:
> 
> > On Mon, Feb 28, 2005 at 04:06:14PM +0100, colbuse@ensisun.imag.fr wrote:
> >
> > > +               /* Setting par[]'s elems at 0.  */
> > > +               memset(par, 0, NPAR*sizeof(unsigned int));
> >
> > No need for the comment here, everybody understands C.
> 
> 
> I knew this code would be understood, but I like comments :-) .
> 
> Well, without it, it gives :
> 
> 
> 
> --- old/drivers/char/vt.c 2004-12-24 22:35:25.000000000 +0100
> +++ new/drivers/char/vt.c 2005-02-28 18:19:11.782717810 +0100
> @@ -1655,8 +1655,8 @@
> vc_state = ESnormal;
> return;
> case ESsquare:
> - for(npar = 0 ; npar < NPAR ; npar++)
> - par[npar] = 0;
> + memset(par, 0, NPAR*sizeof(unsigned int));
>...
> Any other comments/remarks, or is _this_ patch version acceptable?

- whitespace damage
- your sizeof is extremely fragile
  why don't you run sizeof on the array?
- please do any development against -mm
  you'll note that the code you are working against has significantely
  changed in -mm

> Emmanuel Colbus

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

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

[colbuse]

>On Mon, Feb 28, 2005 at 01:57:59PM +0100, colbuse@xxxxxxxxxxxxxxx wrote:
>> Please _don't_ apply this, but tell me what you think about it.

>It's broken. 8)

>> --- 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 < NPAR; npar--)

>How many times do you want this for loop to run?


NPAR times :-). As I stated, npar is unsigned.


See : 
$cat x.c
#include <stdio.h>
#define NPAR 5

int main(void){
        unsigned int i;
        int j=0;
        for(i=NPAR-1;i<NPAR;i--) j++;
        fprintf(stdout,"it has runned %d times :-)\n",j);
}

$gcc x.c -o x && ./x
it has runned 5 times :-)
$

--
Emmanuel Colbus
Club GNU/Linux
ENSIMAG - Departement telecoms

-------------------------------------------------
envoyé via Webmail/IMAG !

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

[Russell King]

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.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

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

[colbuse]

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 :) . 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++;
-               npar = 0;
                vc_state = ESgetpars;
                if (c == '[') { /* Function key */
                        vc_state=ESfunckey;


--- old/include/linux/console_struct.h  2004-12-24 22:33:48.000000000 +0100
+++ old/include/linux/console_struct.h  2005-02-28 14:49:53.653616335 +0100
@@ -44,7 +44,8 @@
        unsigned short  vc_video_erase_char;    /* Background erase character */
        /* VT terminal data */
        unsigned int    vc_state;               /* Escape sequence parser state */
-       unsigned int    vc_npar,vc_par[NPAR];   /* Parameters of current escape
sequence */
+       unsigned int    vc_par[NPAR];           /* Parameters of current escape
sequence */
+       int             vc_npar;                /* Current position in vc_par */
        struct tty_struct *vc_tty;              /* TTY we are attached to */
        /* mode flags */
        unsigned int    vc_charset      : 1;    /* Character set G0 / G1 */


--
Emmanuel Colbus
Club GNU/Linux
ENSIMAG - Departement telecoms


-------------------------------------------------
envoyé via Webmail/IMAG !

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

[Arjan van de Ven]

> -               for(npar = 0 ; npar < NPAR ; npar++)
> +               for(npar = NPAR - 1; npar >= 0; npar--)
>                         par[npar] = 0;

if you really want to clean this up.. why not use memset() instead ?

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

[Martin Mares]

Hello!

> I agree :) . 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?

What does it try to solve?  Your version is in no way better than the previous one.
The previous one was more readable and it's quite possible that both
will be compiled to the same sequence of instructions.

Also, as Arjan noted, if you want to improve the code, use memset.

				Have a nice fortnight
-- 
Martin `MJ' Mares   <mj@ucw.cz>   http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
American patent law: two monkeys, fourteen days.

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

[Horst von Brand]

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

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

[Adrian Bunk]

On Mon, Feb 28, 2005 at 02:13:57PM +0100, colbuse@ensisun.imag.fr wrote:
> 
> >On Mon, Feb 28, 2005 at 01:57:59PM +0100, colbuse@xxxxxxxxxxxxxxx wrote:
> >> Please _don't_ apply this, but tell me what you think about it.
> 
> >It's broken. 8)
> 
> >> --- 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 < NPAR; npar--)
> 
> >How many times do you want this for loop to run?
> 
> 
> NPAR times :-). As I stated, npar is unsigned.
>...

The Linux kernel is not the obfuscated C contest.

Your code might be technically correct, but your patch changes a 
perfectly readable line into an unreadable line for no gain.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

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

[Dmitry Torokhov]

On Mon, 28 Feb 2005 14:13:57 +0100, colbuse@ensisun.imag.fr
<colbuse@ensisun.imag.fr> wrote:
> 
> >On Mon, Feb 28, 2005 at 01:57:59PM +0100, colbuse@xxxxxxxxxxxxxxx wrote:
> >> Please _don't_ apply this, but tell me what you think about it.
> 
> >It's broken. 8)
> 
> >> --- 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 < NPAR; npar--)
> 
> >How many times do you want this for loop to run?
> 
> NPAR times :-). As I stated, npar is unsigned.
> 

for (npar = NPAR - 1; npar >= 0; npar--)

would be more readable and may be even faster on a dumb compiler than
your variant. Still, I'd have compiler worry about such
micro-optimizations.

-- 
Dmitry

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

[Arjan van de Ven]

\
> > >> + for(npar = NPAR-1; npar < NPAR; npar--)
> > 
> > >How many times do you want this for loop to run?
> > 
> > NPAR times :-). As I stated, npar is unsigned.
> > 
> 
> for (npar = NPAR - 1; npar >= 0; npar--)
> 
> would be more readable and may be even faster on a dumb compiler than
> your variant. Still, I'd have compiler worry about such
> micro-optimizations.

actually that goes wrong for npar unsigned...

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

[Dmitry Torokhov]

On Mon, 28 Feb 2005 15:02:32 +0100, Arjan van de Ven
<arjan@infradead.org> wrote:
> \
> > > >> + for(npar = NPAR-1; npar < NPAR; npar--)
> > >
> > > >How many times do you want this for loop to run?
> > >
> > > NPAR times :-). As I stated, npar is unsigned.
> > >
> >
> > for (npar = NPAR - 1; npar >= 0; npar--)
> >
> > would be more readable and may be even faster on a dumb compiler than
> > your variant. Still, I'd have compiler worry about such
> > micro-optimizations.
> 
> actually that goes wrong for npar unsigned...
> 
> 

Oops, you are right... 

-- 
Dmitry

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

[colbuse]

We could change an affectation into an incrementation by this patch, and,
so far I know, incrementing is quicker than or as quick as setting
a variable (depends on the architecture).

Please _don't_ apply this, but tell me what you think about it.
Note that npar is unsigned.

Signed-off-by: Emmanuel Colbus <emmanuel.colbus@ensimag.imag.fr>

--- 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 < NPAR; npar--)
                        par[npar] = 0;
+               npar++;
-               npar = 0;
                vc_state = ESgetpars;
                if (c == '[') { /* Function key */
                        vc_state=ESfunckey;


--
Emmanuel Colbus
Club GNU/Linux
ENSIMAG - departement telecoms


-------------------------------------------------
envoyé via Webmail/IMAG !

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

[Russell King]

On Mon, Feb 28, 2005 at 01:57:59PM +0100, colbuse@ensisun.imag.fr wrote:
> Please _don't_ apply this, but tell me what you think about it.

It's broken. 8)

> --- 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 < NPAR; npar--)

How many times do you want this for loop to run?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

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

[linux-os]

On Mon, 28 Feb 2005 colbuse@ensisun.imag.fr wrote:

> We could change an affectation into an incrementation by this patch, and,
> so far I know, incrementing is quicker than or as quick as setting
> a variable (depends on the architecture).
>
> Please _don't_ apply this, but tell me what you think about it.
> Note that npar is unsigned.
>
> Signed-off-by: Emmanuel Colbus <emmanuel.colbus@ensimag.imag.fr>
>

Since it is common practice to write code as:
                for(npar = 0 ; npar < NPAR ; npar++)
... it is quite likely that the compiler does a better job with
that code than what you substitute. And, if you did a check
of the code generation, it might be different between different
compiler versions.


This kind of "code optimization" won't optimize anything in
the long-run.


> --- 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 < NPAR; npar--)
>                        par[npar] = 0;
> +               npar++;
> -               npar = 0;
>                vc_state = ESgetpars;
>                if (c == '[') { /* Function key */
>                        vc_state=ESfunckey;
>
>
> --
> Emmanuel Colbus
> Club GNU/Linux
> ENSIMAG - departement telecoms
>

Cheers,
Dick Johnson
Penguin : Linux version 2.6.10 on an i686 machine (5537.79 BogoMips).
  Notice : All mail here is now cached for review by Dictator Bush.
                  98.36% of all statistics are fiction.