[patch 2/2] kernel/power/disk.c string fix and if-less iterator

[patch 2/2] kernel/power/disk.c string fix and if-less iterator

[domen]

[-- Attachment #1: string-kernel_power_disk --]
[-- Type: text/plain, Size: 1098 bytes --]

From: Ricardo Nabinger Sanchez <rnsanchez@terra.com.br>



The attached patch:

o  Fixes kernel/power/disk.c string declared as 'char *p = "...";' to be
   declared as 'char p[] = "...";', as pointed by Jeff Garzik.

o  Replaces:
	i++:
	if (i > 3) i = 0;

   By:
	i = (i + 1) % (sizeof(p) - 1);

   Which is if-less, and the adjust value is evaluated by the compiler in
   compile-time in case the string related to this loop is modified.


---
 disk.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

Index: quilt/kernel/power/disk.c
===================================================================
--- quilt.orig/kernel/power/disk.c
+++ quilt/kernel/power/disk.c
@@ -91,15 +91,13 @@ static void free_some_memory(void)
 	unsigned int i = 0;
 	unsigned int tmp;
 	unsigned long pages = 0;
-	char *p = "-\\|/";
+	char p[] = "-\\|/";
 
 	printk("Freeing memory...  ");
 	while ((tmp = shrink_all_memory(10000))) {
 		pages += tmp;
 		printk("\b%c", p[i]);
-		i++;
-		if (i > 3)
-			i = 0;
+		i = (i + 1) % (sizeof(p) - 1);
 	}
 	printk("\bdone (%li pages freed)\n", pages);
 }

--

Re: [patch 2/2] kernel/power/disk.c string fix and if-less iterator

[Pavel Machek]

Hi!

> The attached patch:
> 
> o  Fixes kernel/power/disk.c string declared as 'char *p = "...";' to be
>    declared as 'char p[] = "...";', as pointed by Jeff Garzik.

? Why was char *p ... wrong? Because you could not do sizeof() later?

> o  Replaces:
> 	i++:
> 	if (i > 3) i = 0;
> 
>    By:
> 	i = (i + 1) % (sizeof(p) - 1);
> 
>    Which is if-less, and the adjust value is evaluated by the compiler in
>    compile-time in case the string related to this loop is modified.

Well, why not...
								Pavel

> Index: quilt/kernel/power/disk.c
> ===================================================================
> --- quilt.orig/kernel/power/disk.c
> +++ quilt/kernel/power/disk.c
> @@ -91,15 +91,13 @@ static void free_some_memory(void)
>  	unsigned int i = 0;
>  	unsigned int tmp;
>  	unsigned long pages = 0;
> -	char *p = "-\\|/";
> +	char p[] = "-\\|/";
>  
>  	printk("Freeing memory...  ");
>  	while ((tmp = shrink_all_memory(10000))) {
>  		pages += tmp;
>  		printk("\b%c", p[i]);
> -		i++;
> -		if (i > 3)
> -			i = 0;
> +		i = (i + 1) % (sizeof(p) - 1);
>  	}
>  	printk("\bdone (%li pages freed)\n", pages);
>  }

-- 
teflon -- maybe it is a trademark, but it should not be.

Re: [patch 2/2] kernel/power/disk.c string fix and if-less iterator

[Andreas Schwab]

Pavel Machek <pavel@suse.cz> writes:

> Hi!
>
>> The attached patch:
>> 
>> o  Fixes kernel/power/disk.c string declared as 'char *p = "...";' to be
>>    declared as 'char p[] = "...";', as pointed by Jeff Garzik.

'static const char p[] = "...";' would be even better.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [patch 2/2] kernel/power/disk.c string fix and if-less iterator

[Jörn Engel]

On Mon, 20 June 2005 23:57:13 +0200, domen@coderock.org wrote:
> 
> Index: quilt/kernel/power/disk.c
> ===================================================================
> --- quilt.orig/kernel/power/disk.c
> +++ quilt/kernel/power/disk.c
> @@ -91,15 +91,13 @@ static void free_some_memory(void)
>  	unsigned int i = 0;
>  	unsigned int tmp;
>  	unsigned long pages = 0;
> -	char *p = "-\\|/";
> +	char p[] = "-\\|/";
>  
>  	printk("Freeing memory...  ");
>  	while ((tmp = shrink_all_memory(10000))) {
>  		pages += tmp;
>  		printk("\b%c", p[i]);
> -		i++;
> -		if (i > 3)
> -			i = 0;
> +		i = (i + 1) % (sizeof(p) - 1);
>  	}
>  	printk("\bdone (%li pages freed)\n", pages);
>  }

Isn't "-\\|/" NUL-terminated and hence 5 characters long?  In that
case, you patch may do funny things.

Jörn

-- 
I've never met a human being who would want to read 17,000 pages of
documentation, and if there was, I'd kill him to get him out of the
gene pool.
-- Joseph Costello

Re: [patch 2/2] kernel/power/disk.c string fix and if-less iterator

[Jörn Engel]

On Tue, 21 June 2005 00:10:41 +0200, Pavel Machek wrote:
> 
> > The attached patch:
> > 
> > o  Fixes kernel/power/disk.c string declared as 'char *p = "...";' to be
> >    declared as 'char p[] = "...";', as pointed by Jeff Garzik.
> 
> ? Why was char *p ... wrong? Because you could not do sizeof() later?

Not necessarily wrong, but iirc, "*p" can waste 4 bytes (or 8, for
64-bit platforms).  Since this variable isn't static, that's a
non-issue, but I've seen it on some kernel-janitor list anyway.

Jörn

-- 
Those who come seeking peace without a treaty are plotting.
-- Sun Tzu

Re: [patch 2/2] kernel/power/disk.c string fix and if-less iterator

[Domen Puncer]

On 21/06/05 00:10 +0200, Pavel Machek wrote:
> Hi!
> 
> > The attached patch:
> > 
> > o  Fixes kernel/power/disk.c string declared as 'char *p = "...";' to be
> >    declared as 'char p[] = "...";', as pointed by Jeff Garzik.
> 
> ? Why was char *p ... wrong? Because you could not do sizeof() later?

It usualy generates shorter binary, because there's no pointer variable.
Doesn't seem so in this case:
	   text    data     bss     dec     hex filename
before:	   2333     389       8    2730     aaa kernel/power/disk.o
after:	   2349     389       8    2746     aba kernel/power/disk.o

static const char[] makes it same, so no real gain there either :-(

Sorry for the noise.


	Domen

Re: [patch 2/2] kernel/power/disk.c string fix and if-less iterator

[Mitchell Blank Jr]

J?rn Engel wrote:
> > -	char *p = "-\\|/";
> > +	char p[] = "-\\|/";
> >  
> >  	printk("Freeing memory...  ");
> >  	while ((tmp = shrink_all_memory(10000))) {
> >  		pages += tmp;
> >  		printk("\b%c", p[i]);
> > -		i++;
> > -		if (i > 3)
> > -			i = 0;
> > +		i = (i + 1) % (sizeof(p) - 1);
> >  	}
> >  	printk("\bdone (%li pages freed)\n", pages);
> >  }
> 
> Isn't "-\\|/" NUL-terminated and hence 5 characters long?  In that
> case, you patch may do funny things.

Yeah, you probably really want to do something like:

	static const char p[] = { '-', '\\', '|', '/' };

	printk("Freeing memory...  ");
	while ((tmp = shrink_all_memory(10000))) {
		pages += tmp;
		printk("\b%c", p[i++ % sizeof(p)]);
	}

By using {} instead of "" to declare the char array you avoid placing the
unneeded '\0' terminator on the string.  Plus it definately should be
declared "static".  I don't see any advantage here of keeping "i" explicitly
in range instead of just doing the "i & 3" on each array lookup; it's one
and per loop either way.

-Mitch

Re: [patch 2/2] kernel/power/disk.c string fix and if-less iterator

[Horst von Brand]

Pavel Machek <pavel@suse.cz> wrote:
> domen@coderock.org said:
> > The attached patch:

> > o  Fixes kernel/power/disk.c string declared as 'char *p = "...";' to be
> >    declared as 'char p[] = "...";', as pointed by Jeff Garzik.
> 
> ? Why was char *p ... wrong? Because you could not do sizeof() later?

Note also that this increases the stack usage of the function and slows it
down, as this means allocating an array and filling it in at call time.

> 
> > o  Replaces:
> > 	i++:
> > 	if (i > 3) i = 0;
> > 
> >    By:
> > 	i = (i + 1) % (sizeof(p) - 1);
> > 
> >    Which is if-less, and the adjust value is evaluated by the compiler in
> >    compile-time in case the string related to this loop is modified.
> 
> Well, why not...

Because for that it could be done with a #define of the string... and it is
not that this could change any time soon, so IMHO a constant 3 + comment
would be fine.
-- 
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