[PATCH 0/3] lib/string.c cleanup

[PATCH 0/3] lib/string.c cleanup

[Jesper Juhl]

Here are 3 small cleanup patches for lib/string.c against 2.6.14-rc2-git3

The first patch does whitespace and CodingStyle cleanups. It removes some 
blank lines, removes some trailing whitespace, adds spaces after commas and 
a few similar changes.

The second patch removes a few pointless  register  keywords. register is 
merely a compiler hint that access to the variable should be optimized, but 
gcc (3.3.6 in my case) generates the exact same code with and without the 
keyword, and even if gcc did something different with register present I think 
it is doubtful we would want to optimize access to these variables - especially
since this is generic library code and there are supposed to be optimized 
versions in asm/ for anything that really matters speed wise.

The third patch removes a few pointless explicit casts. The first two hunks of 
the patch really belongs in patch 1, but I missed them on the first pass and 
instead of redoing all 3 patches I stuck them in this one.


One odd sideeffect of patch 1 is that gcc generates an object file that is 4 
bytes smaller with the patch applied, but it's not obvious to me how it 
manages this saving since the patch only makes whitespace changes. I tried 
doing an objdump -D of string.o before and after the patch and compared the 
output with diff, but that didn't reveal the cause of the 4 saved bytes - odd.
I've included the objdump diff below - as you can see it really shouldn't 
make any difference, but the fact remains that the sizes differ :
  juhl@dragon:~/download/kernel/linux-2.6.14-rc2-git3$ ls -l lib/string.o.orig
  -rw-r--r--  1 juhl users 60912 2005-09-23 22:25 lib/string.o.orig
  juhl@dragon:~/download/kernel/linux-2.6.14-rc2-git3$ ls -l lib/string.o
  -rw-r--r--  1 juhl users 60908 2005-09-23 23:05 lib/string.o


 Disassembly of section .text:

@@ -9,46 +9,46 @@
    3:  57                      push   %edi
    4:  89 d7                   mov    %edx,%edi
    6:  56                      push   %esi
-   7:  89 ce                   mov    %ecx,%esi
-   9:  31 c9                   xor    %ecx,%ecx
+   7:  31 d2                   xor    %edx,%edx
+   9:  89 ce                   mov    %ecx,%esi
    b:  53                      push   %ebx
    c:  31 db                   xor    %ebx,%ebx
    e:  83 ec 04                sub    $0x4,%esp
-  11:  85 f6                   test   %esi,%esi
+  11:  85 c9                   test   %ecx,%ecx
   13:  89 45 f0                mov    %eax,0xfffffff0(%ebp)
   16:  74 48                   je     60 <strnicmp+0x60>
   18:  8b 45 f0                mov    0xfffffff0(%ebp),%eax
   1b:  0f b6 1f                movzbl (%edi),%ebx
   1e:  47                      inc    %edi
-  1f:  0f b6 08                movzbl (%eax),%ecx
+  1f:  0f b6 10                movzbl (%eax),%edx
   22:  40                      inc    %eax
   23:  89 45 f0                mov    %eax,0xfffffff0(%ebp)
-  26:  84 c9                   test   %cl,%cl
+  26:  84 d2                   test   %dl,%dl
   28:  74 36                   je     60 <strnicmp+0x60>
   2a:  84 db                   test   %bl,%bl
   2c:  74 32                   je     60 <strnicmp+0x60>
-  2e:  38 d9                   cmp    %bl,%cl
+  2e:  38 da                   cmp    %bl,%dl
   30:  74 2a                   je     5c <strnicmp+0x5c>
-  32:  0f b6 c1                movzbl %cl,%eax
-  35:  89 ca                   mov    %ecx,%edx
+  32:  0f b6 c2                movzbl %dl,%eax
+  35:  89 d1                   mov    %edx,%ecx
   37:  f6 80 00 00 00 00 01    testb  $0x1,0x0(%eax)
   3e:  74 03                   je     43 <strnicmp+0x43>
-  40:  8d 51 20                lea    0x20(%ecx),%edx
+  40:  8d 4a 20                lea    0x20(%edx),%ecx
   43:  0f b6 c3                movzbl %bl,%eax
-  46:  89 d1                   mov    %edx,%ecx
-  48:  89 da                   mov    %ebx,%edx
+  46:  89 ca                   mov    %ecx,%edx
+  48:  89 d9                   mov    %ebx,%ecx
   4a:  f6 80 00 00 00 00 01    testb  $0x1,0x0(%eax)
   51:  74 03                   je     56 <strnicmp+0x56>
-  53:  8d 53 20                lea    0x20(%ebx),%edx
-  56:  38 d1                   cmp    %dl,%cl
-  58:  89 d3                   mov    %edx,%ebx
+  53:  8d 4b 20                lea    0x20(%ebx),%ecx
+  56:  38 ca                   cmp    %cl,%dl
+  58:  89 cb                   mov    %ecx,%ebx
   5a:  75 04                   jne    60 <strnicmp+0x60>
   5c:  4e                      dec    %esi
   5d:  75 b9                   jne    18 <strnicmp+0x18>
   5f:  90                      nop
   60:  83 c4 04                add    $0x4,%esp
-  63:  0f b6 d3                movzbl %bl,%edx
-  66:  0f b6 c1                movzbl %cl,%eax
+  63:  0f b6 c2                movzbl %dl,%eax
+  66:  0f b6 d3                movzbl %bl,%edx
   69:  5b                      pop    %ebx
   6a:  5e                      pop    %esi
   6b:  5f                      pop    %edi



Anyway, the patches have been compile tested, and a kernel with all 3 applied 
boots just fine with no observable defects.


-- 
Jesper Juhl <jesper.juhl@gmail.com>

[PATCH 1/3] lib/string.c cleanup : whitespace and CodingStyle cleanups

[Jesper Juhl]

Whitespace and CodingStyle cleanups for lib/string.c

Removes some blank lines, removes some trailing whitespace, adds spaces 
after commas and a few similar changes.

Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
--- 

 lib/string.c |  113 +++++++++++++++++++++++++++--------------------------------
 1 files changed, 53 insertions(+), 60 deletions(-)

--- linux-2.6.14-rc2-git3/lib/string.c.orig	2005-09-23 22:25:47.000000000 +0200
+++ linux-2.6.14-rc2-git3/lib/string.c	2005-09-23 22:39:57.000000000 +0200
@@ -39,8 +39,10 @@ int strnicmp(const char *s1, const char 
 	c1 = 0;	c2 = 0;
 	if (len) {
 		do {
-			c1 = *s1; c2 = *s2;
-			s1++; s2++;
+			c1 = *s1;
+			c2 = *s2;
+			s1++;
+			s2++;
 			if (!c1)
 				break;
 			if (!c2)
@@ -55,7 +57,6 @@ int strnicmp(const char *s1, const char 
 	}
 	return (int)c1 - (int)c2;
 }
-
 EXPORT_SYMBOL(strnicmp);
 #endif
 
@@ -66,7 +67,7 @@ EXPORT_SYMBOL(strnicmp);
  * @src: Where to copy the string from
  */
 #undef strcpy
-char * strcpy(char * dest,const char *src)
+char *strcpy(char *dest, const char *src)
 {
 	char *tmp = dest;
 
@@ -91,12 +92,13 @@ EXPORT_SYMBOL(strcpy);
  * count, the remainder of @dest will be padded with %NUL.
  *
  */
-char * strncpy(char * dest,const char *src,size_t count)
+char *strncpy(char *dest, const char *src, size_t count)
 {
 	char *tmp = dest;
 
 	while (count) {
-		if ((*tmp = *src) != 0) src++;
+		if ((*tmp = *src) != 0)
+			src++;
 		tmp++;
 		count--;
 	}
@@ -122,7 +124,7 @@ size_t strlcpy(char *dest, const char *s
 	size_t ret = strlen(src);
 
 	if (size) {
-		size_t len = (ret >= size) ? size-1 : ret;
+		size_t len = (ret >= size) ? size - 1 : ret;
 		memcpy(dest, src, len);
 		dest[len] = '\0';
 	}
@@ -138,7 +140,7 @@ EXPORT_SYMBOL(strlcpy);
  * @src: The string to append to it
  */
 #undef strcat
-char * strcat(char * dest, const char * src)
+char *strcat(char *dest, const char *src)
 {
 	char *tmp = dest;
 
@@ -162,7 +164,7 @@ EXPORT_SYMBOL(strcat);
  * Note that in contrast to strncpy, strncat ensures the result is
  * terminated.
  */
-char * strncat(char *dest, const char *src, size_t count)
+char *strncat(char *dest, const char *src, size_t count)
 {
 	char *tmp = dest;
 
@@ -176,7 +178,6 @@ char * strncat(char *dest, const char *s
 			}
 		}
 	}
-
 	return tmp;
 }
 EXPORT_SYMBOL(strncat);
@@ -216,7 +217,7 @@ EXPORT_SYMBOL(strlcat);
  * @ct: Another string
  */
 #undef strcmp
-int strcmp(const char * cs,const char * ct)
+int strcmp(const char *cs, const char *ct)
 {
 	register signed char __res;
 
@@ -224,7 +225,6 @@ int strcmp(const char * cs,const char * 
 		if ((__res = *cs - *ct++) != 0 || !*cs++)
 			break;
 	}
-
 	return __res;
 }
 EXPORT_SYMBOL(strcmp);
@@ -237,7 +237,7 @@ EXPORT_SYMBOL(strcmp);
  * @ct: Another string
  * @count: The maximum number of bytes to compare
  */
-int strncmp(const char * cs,const char * ct,size_t count)
+int strncmp(const char *cs, const char *ct, size_t count)
 {
 	register signed char __res = 0;
 
@@ -246,7 +246,6 @@ int strncmp(const char * cs,const char *
 			break;
 		count--;
 	}
-
 	return __res;
 }
 EXPORT_SYMBOL(strncmp);
@@ -258,12 +257,12 @@ EXPORT_SYMBOL(strncmp);
  * @s: The string to be searched
  * @c: The character to search for
  */
-char * strchr(const char * s, int c)
+char *strchr(const char *s, int c)
 {
-	for(; *s != (char) c; ++s)
+	for (; *s != (char)c; ++s)
 		if (*s == '\0')
 			return NULL;
-	return (char *) s;
+	return (char *)s;
 }
 EXPORT_SYMBOL(strchr);
 #endif
@@ -274,7 +273,7 @@ EXPORT_SYMBOL(strchr);
  * @s: The string to be searched
  * @c: The character to search for
  */
-char * strrchr(const char * s, int c)
+char *strrchr(const char *s, int c)
 {
        const char *p = s + strlen(s);
        do {
@@ -296,8 +295,8 @@ EXPORT_SYMBOL(strrchr);
 char *strnchr(const char *s, size_t count, int c)
 {
 	for (; count-- && *s != '\0'; ++s)
-		if (*s == (char) c)
-			return (char *) s;
+		if (*s == (char)c)
+			return (char *)s;
 	return NULL;
 }
 EXPORT_SYMBOL(strnchr);
@@ -308,7 +307,7 @@ EXPORT_SYMBOL(strnchr);
  * strlen - Find the length of a string
  * @s: The string to be sized
  */
-size_t strlen(const char * s)
+size_t strlen(const char *s)
 {
 	const char *sc;
 
@@ -325,7 +324,7 @@ EXPORT_SYMBOL(strlen);
  * @s: The string to be sized
  * @count: The maximum number of bytes to search
  */
-size_t strnlen(const char * s, size_t count)
+size_t strnlen(const char *s, size_t count)
 {
 	const char *sc;
 
@@ -358,7 +357,6 @@ size_t strspn(const char *s, const char 
 			return count;
 		++count;
 	}
-
 	return count;
 }
 
@@ -384,9 +382,8 @@ size_t strcspn(const char *s, const char
 		}
 		++count;
 	}
-
 	return count;
-}	
+}
 EXPORT_SYMBOL(strcspn);
 
 #ifndef __HAVE_ARCH_STRPBRK
@@ -395,14 +392,14 @@ EXPORT_SYMBOL(strcspn);
  * @cs: The string to be searched
  * @ct: The characters to search for
  */
-char * strpbrk(const char * cs,const char * ct)
+char *strpbrk(const char *cs, const char *ct)
 {
-	const char *sc1,*sc2;
+	const char *sc1, *sc2;
 
-	for( sc1 = cs; *sc1 != '\0'; ++sc1) {
-		for( sc2 = ct; *sc2 != '\0'; ++sc2) {
+	for (sc1 = cs; *sc1 != '\0'; ++sc1) {
+		for (sc2 = ct; *sc2 != '\0'; ++sc2) {
 			if (*sc1 == *sc2)
-				return (char *) sc1;
+				return (char *)sc1;
 		}
 	}
 	return NULL;
@@ -422,9 +419,10 @@ EXPORT_SYMBOL(strpbrk);
  * of that name. In fact, it was stolen from glibc2 and de-fancy-fied.
  * Same semantics, slimmer shape. ;)
  */
-char * strsep(char **s, const char *ct)
+char *strsep(char **s, const char *ct)
 {
-	char *sbegin = *s, *end;
+	char *sbegin = *s;
+	char *end;
 
 	if (sbegin == NULL)
 		return NULL;
@@ -433,10 +431,8 @@ char * strsep(char **s, const char *ct)
 	if (end)
 		*end++ = '\0';
 	*s = end;
-
 	return sbegin;
 }
-
 EXPORT_SYMBOL(strsep);
 #endif
 
@@ -449,13 +445,12 @@ EXPORT_SYMBOL(strsep);
  *
  * Do not use memset() to access IO space, use memset_io() instead.
  */
-void * memset(void * s,int c,size_t count)
+void *memset(void *s, int c, size_t count)
 {
-	char *xs = (char *) s;
+	char *xs = (char *)s;
 
 	while (count--)
 		*xs++ = c;
-
 	return s;
 }
 EXPORT_SYMBOL(memset);
@@ -471,13 +466,13 @@ EXPORT_SYMBOL(memset);
  * You should not use this function to access IO space, use memcpy_toio()
  * or memcpy_fromio() instead.
  */
-void * memcpy(void * dest,const void *src,size_t count)
+void *memcpy(void *dest, const void *src, size_t count)
 {
-	char *tmp = (char *) dest, *s = (char *) src;
+	char *tmp = (char *)dest;
+	char *s = (char *)src;
 
 	while (count--)
 		*tmp++ = *s++;
-
 	return dest;
 }
 EXPORT_SYMBOL(memcpy);
@@ -492,23 +487,21 @@ EXPORT_SYMBOL(memcpy);
  *
  * Unlike memcpy(), memmove() copes with overlapping areas.
  */
-void * memmove(void * dest,const void *src,size_t count)
+void *memmove(void *dest, const void *src, size_t count)
 {
 	char *tmp, *s;
 
 	if (dest <= src) {
-		tmp = (char *) dest;
-		s = (char *) src;
+		tmp = (char *)dest;
+		s = (char *)src;
 		while (count--)
 			*tmp++ = *s++;
-		}
-	else {
-		tmp = (char *) dest + count;
-		s = (char *) src + count;
+	} else {
+		tmp = (char *)dest + count;
+		s = (char *)src + count;
 		while (count--)
 			*--tmp = *--s;
-		}
-
+	}
 	return dest;
 }
 EXPORT_SYMBOL(memmove);
@@ -522,12 +515,12 @@ EXPORT_SYMBOL(memmove);
  * @count: The size of the area.
  */
 #undef memcmp
-int memcmp(const void * cs,const void * ct,size_t count)
+int memcmp(const void *cs, const void *ct, size_t count)
 {
 	const unsigned char *su1, *su2;
 	int res = 0;
 
-	for( su1 = cs, su2 = ct; 0 < count; ++su1, ++su2, count--)
+	for (su1 = cs, su2 = ct; 0 < count; ++su1, ++su2, count--)
 		if ((res = *su1 - *su2) != 0)
 			break;
 	return res;
@@ -545,17 +538,17 @@ EXPORT_SYMBOL(memcmp);
  * returns the address of the first occurrence of @c, or 1 byte past
  * the area if @c is not found
  */
-void * memscan(void * addr, int c, size_t size)
+void *memscan(void *addr, int c, size_t size)
 {
-	unsigned char * p = (unsigned char *) addr;
+	unsigned char *p = (unsigned char *)addr;
 
 	while (size) {
 		if (*p == c)
-			return (void *) p;
+			return (void *)p;
 		p++;
 		size--;
 	}
-  	return (void *) p;
+  	return (void *)p;
 }
 EXPORT_SYMBOL(memscan);
 #endif
@@ -566,18 +559,18 @@ EXPORT_SYMBOL(memscan);
  * @s1: The string to be searched
  * @s2: The string to search for
  */
-char * strstr(const char * s1,const char * s2)
+char *strstr(const char *s1, const char *s2)
 {
 	int l1, l2;
 
 	l2 = strlen(s2);
 	if (!l2)
-		return (char *) s1;
+		return (char *)s1;
 	l1 = strlen(s1);
 	while (l1 >= l2) {
 		l1--;
-		if (!memcmp(s1,s2,l2))
-			return (char *) s1;
+		if (!memcmp(s1, s2, l2))
+			return (char *)s1;
 		s1++;
 	}
 	return NULL;
@@ -600,7 +593,7 @@ void *memchr(const void *s, int c, size_
 	const unsigned char *p = s;
 	while (n-- != 0) {
         	if ((unsigned char)c == *p++) {
-			return (void *)(p-1);
+			return (void *)(p - 1);
 		}
 	}
 	return NULL;

Re: [PATCH 1/3] lib/string.c cleanup : whitespace and CodingStyle cleanups

[Alexey Dobriyan]

On Fri, Sep 23, 2005 at 11:48:44PM +0200, Jesper Juhl wrote:
> Whitespace and CodingStyle cleanups for lib/string.c
> 
> Removes some blank lines, removes some trailing whitespace, adds spaces 
> after commas and a few similar changes.

> -char * strcpy(char * dest,const char *src)
> +char *strcpy(char *dest, const char *src)
	^^^

Why? Seriously.

Re: [PATCH 1/3] lib/string.c cleanup : whitespace and CodingStyle cleanups

[Jesper Juhl]

On 9/24/05, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> On Fri, Sep 23, 2005 at 11:48:44PM +0200, Jesper Juhl wrote:
> > Whitespace and CodingStyle cleanups for lib/string.c
> >
> > Removes some blank lines, removes some trailing whitespace, adds spaces
> > after commas and a few similar changes.
>
> > -char * strcpy(char * dest,const char *src)
> > +char *strcpy(char *dest, const char *src)
>         ^^^
>
> Why? Seriously.
>

Two reasons

1) it matches the style most commonly used in the kernel

2) when I (and i suspect others) see
   <foo> * <bar>   my first thought is "multiplication"
   but when I see
   <foo> *<bar>   my first thought is "ptr variable" or "function returning ptr"


--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

[PATCH 2/3] lib/string.c cleanup : remove pointless register keyword

[Jesper Juhl]

Removes a few pointless  register  keywords. register is merely a compiler 
hint that access to the variable should be optimized, but gcc (3.3.6 in my 
case) generates the exact same code with and without the keyword, and even 
if gcc did something different with register present I think it is doubtful 
we would want to optimize access to these variables - especially since this 
is generic library code and there are supposed to be optimized versions in 
asm/ for anything that really matters speed wise.


Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

 lib/string.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

--- linux-2.6.14-rc2-git3/lib/string.c-with-patch-1	2005-09-23 22:44:42.000000000 +0200
+++ linux-2.6.14-rc2-git3/lib/string.c	2005-09-23 22:45:35.000000000 +0200
@@ -219,7 +219,7 @@ EXPORT_SYMBOL(strlcat);
 #undef strcmp
 int strcmp(const char *cs, const char *ct)
 {
-	register signed char __res;
+	signed char __res;
 
 	while (1) {
 		if ((__res = *cs - *ct++) != 0 || !*cs++)
@@ -239,7 +239,7 @@ EXPORT_SYMBOL(strcmp);
  */
 int strncmp(const char *cs, const char *ct, size_t count)
 {
-	register signed char __res = 0;
+	signed char __res = 0;
 
 	while (count) {
 		if ((__res = *cs - *ct++) != 0 || !*cs++)

[PATCH 3/3] lib/string.c cleanup : remove pointless explicit casts

[Jesper Juhl]

Remove a few pointless explicit casts.
The first two hunks of the patch really belongs in patch 1, but I missed 
them on the first pass and instead of redoing all 3 patches I stuck them in 
this one.


Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

 lib/string.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

--- linux-2.6.14-rc2-git3/lib/string.c-with-patch-2	2005-09-23 22:46:35.000000000 +0200
+++ linux-2.6.14-rc2-git3/lib/string.c	2005-09-23 23:05:19.000000000 +0200
@@ -36,7 +36,7 @@ int strnicmp(const char *s1, const char 
 	/* Yes, Virginia, it had better be unsigned */
 	unsigned char c1, c2;
 
-	c1 = 0;	c2 = 0;
+	c1 = c2 = 0;
 	if (len) {
 		do {
 			c1 = *s1;
@@ -148,7 +148,6 @@ char *strcat(char *dest, const char *src
 		dest++;
 	while ((*dest++ = *src++) != '\0')
 		;
-
 	return tmp;
 }
 EXPORT_SYMBOL(strcat);
@@ -447,7 +446,7 @@ EXPORT_SYMBOL(strsep);
  */
 void *memset(void *s, int c, size_t count)
 {
-	char *xs = (char *)s;
+	char *xs = s;
 
 	while (count--)
 		*xs++ = c;
@@ -468,8 +467,8 @@ EXPORT_SYMBOL(memset);
  */
 void *memcpy(void *dest, const void *src, size_t count)
 {
-	char *tmp = (char *)dest;
-	char *s = (char *)src;
+	char *tmp = dest;
+	char *s = src;
 
 	while (count--)
 		*tmp++ = *s++;
@@ -492,13 +491,15 @@ void *memmove(void *dest, const void *sr
 	char *tmp, *s;
 
 	if (dest <= src) {
-		tmp = (char *)dest;
-		s = (char *)src;
+		tmp = dest;
+		s = src;
 		while (count--)
 			*tmp++ = *s++;
 	} else {
-		tmp = (char *)dest + count;
-		s = (char *)src + count;
+		tmp = dest;
+		tmp += count;
+		s = src;
+		s += count;
 		while (count--)
 			*--tmp = *--s;
 	}
@@ -540,7 +541,7 @@ EXPORT_SYMBOL(memcmp);
  */
 void *memscan(void *addr, int c, size_t size)
 {
-	unsigned char *p = (unsigned char *)addr;
+	unsigned char *p = addr;
 
 	while (size) {
 		if (*p == c)