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 <[email protected]>
--- 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 !
On Mon, Feb 28, 2005 at 01:57:59PM +0100, [email protected] 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
>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 !
On Mon, Feb 28, 2005 at 02:13:57PM +0100, [email protected] 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
On Mon, Feb 28, 2005 at 02:13:57PM +0100, [email protected] 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
On Mon, 28 Feb 2005 [email protected] 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 <[email protected]>
>
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.
On Mon, 28 Feb 2005 14:13:57 +0100, [email protected]
<[email protected]> 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
\
> > >> + 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...
On Mon, 28 Feb 2005 15:02:32 +0100, Arjan van de Ven
<[email protected]> 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
Surlignage Russell King <[email protected]>:
> On Mon, Feb 28, 2005 at 02:13:57PM +0100, [email protected] 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 !
> - 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 ?
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 <[email protected]> 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.
[email protected] said:
> Surlignage Russell King <[email protected]>:
> > On Mon, Feb 28, 2005 at 02:13:57PM +0100, [email protected] 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
>> - 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 !
On Mon, Feb 28, 2005 at 04:06:14PM +0100, [email protected] 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 <[email protected]>
On Mon, 28 Feb 2005, Stelian Pop wrote:
> On Mon, Feb 28, 2005 at 04:06:14PM +0100, [email protected] 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
On Mon, Feb 28, 2005 at 06:29:39PM +0100, [email protected] 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 <[email protected]>
On Mon, Feb 28, 2005 at 06:29:39PM +0100, [email protected] wrote:
>
>
> On Mon, 28 Feb 2005, Stelian Pop wrote:
>
> > On Mon, Feb 28, 2005 at 04:06:14PM +0100, [email protected] 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