Re: pango patch -- fix some warnings, at least one bug

[Darin Adler <darin bentspoon com>]

On Thursday, July 12, 2001, at 08:49  AM, Owen Taylor wrote:

> > Index: modules/arabic/arabic-x.c
> > ===================================================================
> > RCS file: /cvs/gnome/pango/modules/arabic/arabic-x.c,v
> > retrieving revision 1.21
> > diff -p -u -r1.21 arabic-x.c
> > --- modules/arabic/arabic-x.c	2001/06/25 15:09:39	1.21
> > +++ modules/arabic/arabic-x.c	2001/07/11 23:39:32
> > @@ -178,7 +178,7 @@ arabic_engine_shape (PangoFont        *f
> >                        PangoGlyphString *glyphs)
> >   {
> >       PangoXSubfont   subfont;
> > -    long            n_chars;
> > +    int             n_chars;
> >       int             i;
> >       ArabicFontInfo *fs;
> >       const char     *p;
>
> I think it would be marginally better to change the argument to
> arabic_reshape() instead.

I looked into this, and it's not simple to do. The arabic_reshape function 
passes the int* to functions named shape and doublelig. These do a lot of 
manipulation on the length. Changing the parameter to arabic_reshape is 
basically fixing all the code to handle the length type correctly. 
Changing the local variable to int is going with the flow of what's there.

Given that, are you sure that you want me to tackle the more-ambitious 
change? It's probably only about 10 minutes work, but I'm worried about 
doing it with no way to test if my changes broke anything.

> > Index: modules/tamil/tamil-x.c
> > ===================================================================
> > RCS file: /cvs/gnome/pango/modules/tamil/tamil-x.c,v
> > retrieving revision 1.13
> > diff -p -u -r1.13 tamil-x.c
> > --- modules/tamil/tamil-x.c	2001/06/14 20:38:22	1.13
> > +++ modules/tamil/tamil-x.c	2001/07/11 23:39:33
> > @@ -232,7 +232,7 @@ tamil_engine_shape (PangoFont        *fo
> >     wc = (gunichar *)g_malloc (sizeof(gunichar) * n_chars * 2);
> >
> >     p = text;
> > -  prevchar = 0;complete = 1;/* One character look behind */
> > +  prevchar = 0;currchar = 0;complete = 1;/* One character look behind *
> > /
> >     n_glyph = 0;
> >     cluster_start = text;
>
> Could you do this as:
>
>  gunichar currchar = 0;   /* Quiet compiler */
>
> To indicate that the assignment is just a dummy to suppress a spurious
> warning. (Initializing currchar to 0 doesn't actually make sense - 0
> is not a legitimate value.)

OK. I admit I did consider doing it this way and I did what I did because 
of laziness. I'll fix it before I commit.

    -- Darin