svn commit: r227808 - head/lib/libc/string

Bruce Evans brde at optusnet.com.au
Tue Nov 22 09:53:57 UTC 2011


On Tue, 22 Nov 2011, Eitan Adler wrote:

> Log:
>  - add check for pointer equality prior to performing the O(n) pass

This seems like a negative optimization.  The pointers are unequal in
the usual case.  More seriously, it gives worse undefined behaviour
when the pointers are invalid.  E.g., strcmp(NULL, NULL) now returns
0 where it used to trap for the null pointer access.

This has many style bugs.

>  - while here change 's' to 's1' in strcoll
>
>  Submitted by:	eadler@
>  Reviewed by:	theraven@
>  Approved by:	brooks@
>  MFC after:	2 weeks

> Modified: head/lib/libc/string/strcasecmp.c
> ==============================================================================
> --- head/lib/libc/string/strcasecmp.c	Mon Nov 21 23:32:14 2011	(r227807)
> +++ head/lib/libc/string/strcasecmp.c	Tue Nov 22 00:07:53 2011	(r227808)
> @@ -48,6 +48,9 @@ strcasecmp_l(const char *s1, const char
> 	const u_char
> 			*us1 = (const u_char *)s1,
> 			*us2 = (const u_char *)s2;
> +	if (s1 == s2)
> +	    return (0);
> +

This adds non-KNF indentation (3 spaces instead of 1 tab) and an extra
blank line.

Previous locale changes have already mangled this file, with additions
of extra blank lines and worse.

> 	FIX_LOCALE(locale);
>
> 	while (tolower_l(*us1, locale) == tolower_l(*us2++, locale))
> @@ -65,18 +68,21 @@ int
> strncasecmp_l(const char *s1, const char *s2, size_t n, locale_t locale)
> {
> 	FIX_LOCALE(locale);
> -	if (n != 0) {
> -		const u_char
> -				*us1 = (const u_char *)s1,
> -				*us2 = (const u_char *)s2;
> -
> -		do {
> -			if (tolower_l(*us1, locale) != tolower_l(*us2++, locale))
> -				return (tolower_l(*us1, locale) - tolower_l(*--us2, locale));
> -			if (*us1++ == '\0')
> -				break;
> -		} while (--n != 0);
> -	}
> +
> +	const u_char
> +			*us1 = (const u_char *)s1,
> +			*us2 = (const u_char *)s2;
> +

This adds a dependency on C99 by placing the declarations after a statement.
Moving the declarations out of an inner block is otherwise good.

> +	if (( s1 == s2) | (n == 0))

This adds an extra space, a weird operator (arithmetic OR instead of
logical OR), and extra parentheses.  The extra parentheses are more needed
with the weird operator (arithmetic OR has a weird precedence that happens
to make the parentheses unnecessary here, but compilers should warn about
the relative precedences being surprising and force you to add the
parentheses).

> +	    return (0);

This adds the usual non-KNF indentation.

> +
> +

This adds 2 extra blank lines instead of only 1.

> +	do {
> +		if (tolower_l(*us1, locale) != tolower_l(*us2++, locale))
> +			return (tolower_l(*us1, locale) - tolower_l(*--us2, locale));
> +		if (*us1++ == '\0')
> +			break;
> +	} while (--n != 0);
> 	return (0);
> }
>
>
> Modified: head/lib/libc/string/strcmp.c
> ==============================================================================
> --- head/lib/libc/string/strcmp.c	Mon Nov 21 23:32:14 2011	(r227807)
> +++ head/lib/libc/string/strcmp.c	Tue Nov 22 00:07:53 2011	(r227808)
> @@ -44,6 +44,9 @@ __FBSDID("$FreeBSD$");
> int
> strcmp(const char *s1, const char *s2)
> {
> +	if (s1 == s2)
> +	    return (0);
> +

This adds non-KNF indentation and an extra blank line, and removes the
strange KNF blank line before the (null) declarations.

> 	while (*s1 == *s2++)
> 		if (*s1++ == '\0')
> 			return (0);
>
> Modified: head/lib/libc/string/strcoll.c

OK.

> Modified: head/lib/libc/string/strncmp.c
> ==============================================================================
> --- head/lib/libc/string/strncmp.c	Mon Nov 21 23:32:14 2011	(r227807)
> +++ head/lib/libc/string/strncmp.c	Tue Nov 22 00:07:53 2011	(r227808)
> @@ -39,8 +39,9 @@ int
> strncmp(const char *s1, const char *s2, size_t n)
> {
>
> -	if (n == 0)
> +	if ((n == 0) | (s1 == s2))
> 		return (0);
> +

As above, but without removing the blank line before the null declarations,
without the extra space before s1, with only 1 extra blank line, and with
the equality tests reversed relative to the above.

> 	do {
> 		if (*s1 != *s2++)
> 			return (*(const unsigned char *)s1 -
>

Bruce


More information about the svn-src-head mailing list