From owner-svn-src-head@FreeBSD.ORG Tue Nov 22 09:53:57 2011 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E2E04106566B; Tue, 22 Nov 2011 09:53:56 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail15.syd.optusnet.com.au (mail15.syd.optusnet.com.au [211.29.132.196]) by mx1.freebsd.org (Postfix) with ESMTP id 6D6208FC08; Tue, 22 Nov 2011 09:53:55 +0000 (UTC) Received: from c211-28-227-231.carlnfd1.nsw.optusnet.com.au (c211-28-227-231.carlnfd1.nsw.optusnet.com.au [211.28.227.231]) by mail15.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id pAM9rpHj027768 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 22 Nov 2011 20:53:53 +1100 Date: Tue, 22 Nov 2011 20:53:51 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Eitan Adler In-Reply-To: <201111220007.pAM07rjP065608@svn.freebsd.org> Message-ID: <20111122195444.N8431@besplex.bde.org> References: <201111220007.pAM07rjP065608@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r227808 - head/lib/libc/string X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 22 Nov 2011 09:53:57 -0000 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