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