373010 - Mingw build failure in mozilla/gfx/thebes/src/gfxWindowsFonts.cpp

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Bengt-Erik Soderstrom]

I believe this is a regression from the last check-in for bug 372636. Now I get this:

d:/mozilla/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp: In member function `HFONT
__* gfxWindowsFont::MakeHFONT()':
d:/mozilla/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp:232: warning: comparison b
etween signed and unsigned integer expressions
d:/mozilla/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp: In function `PRBool IsMis
singGlyphsGDI(HDC__*, const char*, PRUint32, WCHAR*)':
d:/mozilla/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp:647: error: invalid conver
sion from `WCHAR*' to `WORD*'
d:/mozilla/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp: In function `PRBool IsMis
singGlyphsGDI(HDC__*, const PRUnichar*, PRUint32, WCHAR*)':
d:/mozilla/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp:664: error: invalid conver
sion from `WCHAR*' to `WORD*'
d:/mozilla/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp: In constructor `Uniscribe
Item::UniscribeItem(gfxContext*, HDC__*, const PRUnichar*, PRUint32, SCRIPT_ITEM
*, gfxWindowsFontGroup*)':
d:/mozilla/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp:1425: warning: `UniscribeI
tem::mAlternativeString' will be initialized after
d:/mozilla/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp:1419: warning:   `SCRIPT_I
TEM*UniscribeItem::mScriptItem'
d:/mozilla/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp:901: warning:   when initi
alized here
d:/mozilla/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp:1434: warning: `UniscribeI
tem::mNumGlyphs' will be initialized after
d:/mozilla/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp:1433: warning:   `int Unis
cribeItem::mMaxGlyphs'
d:/mozilla/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp:901: warning:   when initi
alized here
d:/mozilla/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp: In member function `void
gfxWindowsFontGroup::InitTextRunUniscribe(gfxContext*, gfxTextRun*, const PRUnic
har*, PRUint32)':
d:/mozilla/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp:1537: warning: unused vari
able 'fontIndex'
../../../dist/include/thebes/gfxWindowsFonts.h: At global scope:
../../../dist/include/thebes/gfxWindowsFonts.h:232: warning: 'PRUint8 CharRangeB
it(PRUint32)' defined but not used
make[6]: *** [gfxWindowsFonts.o] Error 1
make[6]: Leaving directory `/cygdrive/d/mozilla/mozilla/object-mingw/gfx/thebes/
src'
make[5]: *** [libs] Error 2
make[5]: Leaving directory `/cygdrive/d/mozilla/mozilla/object-mingw/gfx/thebes'

make[4]: *** [libs] Error 2
make[4]: Leaving directory `/cygdrive/d/mozilla/mozilla/object-mingw/gfx'
make[3]: *** [libs_tier_gecko] Error 2
make[3]: Leaving directory `/cygdrive/d/mozilla/mozilla/object-mingw'
make[2]: *** [tier_gecko] Error 2
make[2]: Leaving directory `/cygdrive/d/mozilla/mozilla/object-mingw'
make[1]: *** [alldep] Error 2
make[1]: Leaving directory `/cygdrive/d/mozilla/mozilla/object-mingw'
make: *** [alldep] Error 2

To make sure I backed out one step on gfxWindowsFonts.cpp (1.88 -> 1.87) and then it built just fine.

Comment 1

[Martijn Wargers (dead)]

Attachment: patch?

This seems to fix it for me.
Btw, I don't seem to need the patch for bug 367742 anymore, at least I don't trip anymore over a compiler error without that patch.

Comment 2

[Bengt-Erik Soderstrom]

Yes, Martijn. Your patch works for me too. I simply took the current version (1.88) of gfxWindowsFonts.cpp and applied your patch manually.

I also made a build with VC8, to check that also this worked, and it did.

So, can we please have this reviewed and checked in?

 

Comment 4

[Martijn Wargers (dead)]

Checking in gfxWindowsFonts.cpp;
/cvsroot/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp,v  <--  gfxWindowsFonts.cpp
new revision: 1.89; previous revision: 1.88
done

Checked in on trunk.

Comment 5

[Sergey Fukanchik]

I think this patch is bad example of programming.

Consider the use of function 'IsMissingGlyphsGDI'.
InitTextRunGDI allocates on stack sizeof(WCHAR) * 500 bytes (why 500 and why use nsAutoTArray when WCHAR glyphArray[500] will be enough?) using
nsAutoTArray and immediately reserves sizeof(WCHAR) * aLength bytes from it.

Next auto array pointer is passed to IsMissingGlyphsGDI, and there it is passed
to GetGlyphIndicesA which expects (according to documentation) array of length
aLength of WORDs (indices to glyphs array) - not WCHARS (characters).

Of course sizeof(WCHAR)==sizeof(WORD) and we do not overwrite memory here.

Furthermore, few lines after the call to IsMissingGlyphsGDI in InitTextRunGDI the same buffer is reused for other purpose, and hence value set in IsMissingGlyphsGDI isn't used anywhere except in IsMissingGlyphsGDI. So it is possible to exclude one argument from arglist of IsMissingGlyphsGDI and allocate this buffer in IsMissingGlyphsGDI itself without confusing someone who will look at this code sometime later.

For instance my first thought was that after return of IsMissingGlyphsGDI glyphArray values set there are used somehow, but only return value of this function is used.

Comment 6

[Martijn Wargers (dead)]

Serge, you're more than welcome to write a better patch. Thanks!

Comment 7

[Bengt-Erik Soderstrom]

I don't think the patch as such is an example of bad programming. But maybe, considering the large number of compiler warnings, with gcc, with mingw, with VC8, that indicates that the whole of gfxWindowsFonts.cpp should be looked at. There are certainly room for improvements, I think everyone agrees to that.

But the main issue to get Mingw happy and at the same time not busting anything else, other compilers, other platforms etc is good.
Thanks Martijn for the quick action! 

Comment 8

[Stuart Parmenter]

I keep trying to fix the warnings but they keep coming back.. :/