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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/