[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 5 Feb 2013 12:19:10 +0100
From: Szabolcs Nagy <nsz@...t70.net>
To: musl@...ts.openwall.com
Subject: Re: Re: [PATCH 0/4] Refactor and expand string functions.
* Nathan McSween <nwmcsween@...il.com> [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)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
