Re: memmem issues

[Bruno Haible]

Hi Paul,

Most of your comments apply to all copies of the KMP code in gnulib.

> Eric Blake <address@hidden> 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