From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/2737 Path: news.gmane.org!not-for-mail From: Szabolcs Nagy Newsgroups: gmane.linux.lib.musl.general Subject: Re: Re: [PATCH 0/4] Refactor and expand string functions. Date: Tue, 5 Feb 2013 12:19:10 +0100 Message-ID: <20130205111910.GQ6181@port70.net> References: <1359936735-31915-1-git-send-email-nwmcsween@gmail.com> <511089D1.1000803@gmail.com> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1360063165 10631 80.91.229.3 (5 Feb 2013 11:19:25 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Tue, 5 Feb 2013 11:19:25 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-2738-gllmg-musl=m.gmane.org@lists.openwall.com Tue Feb 05 12:19:42 2013 Return-path: Envelope-to: gllmg-musl@plane.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1U2gYw-0007cN-NM for gllmg-musl@plane.gmane.org; Tue, 05 Feb 2013 12:19:42 +0100 Original-Received: (qmail 29977 invoked by uid 550); 5 Feb 2013 11:19:22 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 29964 invoked from network); 5 Feb 2013 11:19:22 -0000 Content-Disposition: inline In-Reply-To: <511089D1.1000803@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Xref: news.gmane.org gmane.linux.lib.musl.general:2737 Archived-At: * Nathan McSween [2013-02-04 20:25:53 -0800]: > /** > * memchr - Word sized c standard memchr. > * @s: Source > * @c: Character > * @n: Max size of @s > */ > void *memchr(const void *s, int c, size_t n) > { > const unsigned char *cs = (const unsigned char *)s; > const size_t *w; > > c = (unsigned char)c; > > for (; (uintptr_t)cs % sizeof(size_t); cs++, n--) { > if (!n) return NULL; > if (*cs == c) return (void *)cs; > } > > for (w = (const size_t *)cs; !word_has_char(*w, c); w++, n--) > if (!n) return NULL; > for (cs = (const unsigned char *)w; *cs != c; cs++, n--) > i did you test this code? w++ but n-- does not seem right > #define WORD_LSB_ONE ((size_t)-1 / (unsigned char)-1) > #define WORD_MSB_ONE (WORD_LSB_ONE * ((unsigned char)-1 / 2 + 1)) > > /** > * word_has_zero - Word has a zero character > * @w: Word > */ > static inline char word_has_zero(size_t w) > { > return !!((w - WORD_LSB_ONE) & (~w & WORD_MSB_ONE)); > } > > /** > * word_has_char - Word has a character > * @w: Word > */ > static inline char word_has_char(size_t w, char c) > { > return !!((w - WORD_LSB_ONE) > & ((~w & WORD_MSB_ONE)^(WORD_LSB_ONE the code is mostly ok: don't use "char c" argument, use unsigned char or int, same for return value but i don't like the style of this maybe it's just me but the WORD_LSB_ONE is a long name that does not help me understand what it is, it could be W1 vs W128 and the same amount of explanation would be embedded in the name (but much shorter and thus easier to recognize, things fit on one line etc, it assumes 8bit char though) the comments are bad (lot of text with no content) and the code actually does magic that may need explanation it's rather frustrating when you try to understand what's going on and have to read lot of useless comments.. i dont see how word_has_char works, the name suggests that it tests if w has c in it, but that's not what it does (and the comment is actively misleading: only shows one arg, which demonstrates why i dislike these kinds of comments, thye cause more harm than good)