Hi Paul, Most of your comments apply to all copies of the KMP code in gnulib.
> Eric Blake <[EMAIL PROTECTED]> writes: > > + size_t *table = (size_t *) malloca (m * sizeof (size_t)); > > + if (table == NULL) > > + return false; > > Shouldn't this check for overflow in the multiplication? Yes, it should. I'm always lazy about this. > > + unsigned char b = (unsigned char) needle[i - 1]; > > ... > > + if (b == (unsigned char) needle[j]) > > Would it be cleaner to declare 'b' to be of type 'char' and avoid the > casts? No; ISO C 99 section 7.21.4 says that when byte strings are compared the elements are considered as 'unsigned char' values. Why risk bugs when the approach is very simple: after fetching any 'char' from any of the strings, cast it to 'unsigned char'. Simple rule, simple to remember, works fine. > > + if ((unsigned char) needle[j] == (unsigned char) *phaystack) > > Can both casts be omitted? Same answer. > > + return memchr (haystack, (unsigned char) *Needle, haystack_len); > > Can the cast be omitted? Same answer. > > + return (void *) haystack; > > How about "return Haystack;"? That avoids a cast. ... but will not compile when a C++ compiler is used. > (As you can tell, I prefer to avoid casts....) As you can tell, I prefer to write code in the intersection of C and C++. > > + if (needle_len == 0) > > Perhaps a __builtin_expect would be helpful here? Hardly. Nowadays gcc knows that comparison with == against a particular value is "unlikely", AFAIK. No need to help it with __builtin_expect here. Bruno
