Re: [PATCH] (x)memcoll: minor tweaks

[Bruno Haible]

Hi Paul,

There is one API bug and two oddities in your patch:

1) You removed the 'abort' calls in memcoll0 that were intended to verify
   that the caller passes meaningful arguments. The documentation that you
   added:

   /* Compare S1 (with length S1LEN) and S2 (with length S2LEN) according
      to the LC_COLLATE locale.  S1 and S2 must both end in a null byte.
      Set errno to an error number if there is an error, and to zero
      otherwise.  */

   asserts that a valid way to use this function is e.g.

      memcoll0 (s1, strlen (s1), s2, strlen (s2)).

   (By common understanding, the "length" of a C string is strlen (s).)

   But when a user does this, then in strcoll_loop line 46 you
   subtract (strlen (s1) + 1) from s1len which is strlen (s1), and
   the loop overruns the end of the string and crashes.

   I would add back the abort()s as a safeguard. Performance does matter,
   but the time spent in a byte lookup and a conditional branch of probability
   0 is negligible compared to an strcoll() call which does all kinds of
   locale dependent processing.

2) You added documentation to memcoll0 that does not make references to other
   functions. Good. Can we have the same for xmemcoll0 as well?

3) There are two 'const char *' left over in memcoll.c lines 33 and 106.

Bruno