Re: [PATCH] proper lowercasing of multi-octet-char "keys" string

[Julian Foad]

Charles Levert wrote:
> Ok to commit?
>
>
>
> Index: grep/ChangeLog
> ===================================================================
> RCS file: /cvsroot/grep/grep/ChangeLog,v
> retrieving revision 1.272
> diff -u -r1.272 ChangeLog
> --- grep/ChangeLog      27 Sep 2005 14:50:20 -0000      1.272
> +++ grep/ChangeLog      3 Nov 2005 23:28:05 -0000
> @@ -1,3 +1,11 @@
> +2005-11-04  Charles Levert  <address@hidden>

If this fixes any specific bug (especially one recorded in the Savannah 
tracker) it would be good to say so here.

> +       * src/grep.c (mb_icase_keys): New function to properly lowercase
> +         keys if match_icase.  The problem was that some multi-octet
> +         characters can get longer or shorter upon this conversion, so that
> +         it cannot just naively be done in place on the same memory buffer.
> +       * src/grep.c (main): Call mb_icase_keys (and remove in-line code).
> +
>  2005-09-27  Stepan Kasal  <address@hidden>
>  
>  	* doc/grep.1: Fix a typo.
> Index: grep/src/grep.c
> ===================================================================
[...]
> +#ifdef MBS_SUPPORT
> +static void
> +mb_icase_keys (char **keys, size_t *len)

Would you mind adding a comment for this function?  Just a couple of lines 
saying what it does and what its arguments mean.  I know Grep is historically 
short of comments, but I don't see any reason to perpetuate the mystique.

> +{
> +  wchar_t wc;
> +  mbstate_t sti, stj;
> +  size_t i, j, li, lj;
> +  char *ki, *kj;
> +  int mcm;

It took me a few minutes to work out what all the variables are for.  I think 
those with suffix "i" refer to the input (original) string and those with 
suffix "j" refer to the output (new) string.  A little hint would have been 
nice, but perhaps now is not the time to ask you to comment your local variables.

> +
> +  if ((mcm = MB_CUR_MAX) == 1 || !match_icase)
> +    return;

It would be nicer to leave the "match_icase" test in the caller, so that this 
function just does a certain thing and doesn't depend on a global variable. 
(It's not so bad for "main" to depend on a global variable.)

Apart from those minor points, the code looks good, so go ahead and commit it 
with whichever of the above tweaks you are happy to make.

- Julian