kernel/power/disk.c string fix and if-less iterator
Subsystems (2)
PATCH 2/2 kernel/power/disk.c string fix and if-less iterator
From: Ricardo Nabinger Sanchez <[email protected]>
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(-)
kernel/power/disk.c +2 -4
@@ -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); }
--
Hi!
Quoted 4 lines
The attached patch:
o Fixes
kernel/power/disk.cstring 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?
Quoted 9 lines
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
Quoted 22 lines
Index:
quilt/kernel/power/disk.c
===================================================================kernel/power/disk.c +2
-4@@ -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); }
Signature · 1 line
teflon -- maybe it is a trademark, but it should not be.
Quoted 6 lines · Pavel Machek
Hi!
Quoted 4 lines
The attached patch:
o Fixes
kernel/power/disk.cstring declared as 'char *p = "...";' to be declared as 'char p[] = "...";', as pointed by Jeff Garzik.
'static const char p[] = "...";' would be even better.
Andreas.
Signature · 4 lines
Andreas Schwab, SuSE Labs, [email protected] 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."
Quoted 7 lines · Pavel Machek
Quoted 5 lines
The attached patch:
o Fixes
kernel/power/disk.cstring 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
Signature · 2 lines
Those who come seeking peace without a treaty are plotting. -- Sun Tzu
Quoted 8 lines · Pavel Machek
Hi!
Quoted 4 lines
The attached patch:
o Fixes
kernel/power/disk.cstring 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
Quoted 7 lines · Pavel Machek
Quoted 4 lines · [email protected] said:
The attached patch:
o Fixes
kernel/power/disk.cstring 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.
Quoted 5 lines
Quoted 5 lines
o Replaces:
i++: if (i > 3) i = 0;
Quoted 7 lines
Quoted 5 lines
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.
Signature · 4 lines
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
Quoted 13 lines · On Mon, 20 June 2005 23:57:13 +0200, [email protected] wrote:
Index:
quilt/kernel/power/disk.c
===================================================================kernel/power/disk.c +1
-1@@ -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... " );
kernel/power/disk.c static void free_some_memory(void) · line 103
Quoted 10 lines
kernel/power/disk.c +1
-3while ((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
Signature · 4 lines
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
Quoted 17 lines · J?rn Engel
Quoted 4 lines
kernel/power/disk.c +1
-1-char *p ="-\\|/" ; +char p[] ="-\\|/" ;printk ("Freeing memory... " );kernel/power/disk.c static void free_some_memory(void) · line 103
Quoted 10 lines
kernel/power/disk.c +1
-3while ((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