Date: Tue, 22 Nov 2011 20:53:51 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Eitan Adler <eadler@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r227808 - head/lib/libc/string Message-ID: <20111122195444.N8431@besplex.bde.org> In-Reply-To: <201111220007.pAM07rjP065608@svn.freebsd.org> References: <201111220007.pAM07rjP065608@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20111122195444.N8431>