Re: resend add missing glyph code to GetGlyphIndices

[Dmitry Timoshkov]

"Jeff L"  wrote:
> -DWORD WineEngGetGlyphIndices(GdiFont font, LPCWSTR lpstr, INT count,
> -
> - */

Strange indentation at the end of a comment.
> +DWORD WineEngGetGlyphIndices(HDC hdc, GdiFont font, LPCWSTR lpstr, INT count,
>  LPWORD pgi, DWORD flags)
> {
>     INT i;
> -
> - TEXTMETRICW textm;
> for(i = 0; i < count; i++)
> - {
>    pgi[i] = get_glyph_index(font, lpstr[i]);
> -
> -
>    if  (pgi[i] == 0)
> -
>    {
> -
>        if  (flags & GGI_MARK_NONEXISTING_GLYPHS)
> -
>            pgi[i] = 0x001f;                  /* Indicate non existance */
> -
>        else
> -
>        {
> -
>            GetTextMetricsW(hdc, &textm);
> -
>            pgi[i] = textm.tmDefaultChar;
> -
>        }
> -
>    }
> - }
> return count;
>
> }

I'd suggest to move GetTextMetricsW outside of the loop to not kill
the performance.
> - WCHAR    testtext[] = {'T',0x0000,'s','t',0xffff,0};
> - WORD     glyphs[(sizeof(testtext)/2)-1];
> - TEXTMETRIC textm;
> -
> - memset(&lf, 0, sizeof(lf));
> - strcpy(lf.lfFaceName, "Symbol");
> - lf.lfHeight = 20;
> -
> - hfont = CreateFontIndirectA(&lf);
> - hdc = GetDC(0);
> -
> - ok(GetTextMetrics(hdc, &textm), "GetTextMetric failed\n");

Please set tab width to 8 instead of 4 and do not mix tabs and spaces.

resend add missing glyph code to GetGlyphIndices

[Jeff Latimer]

Dmitry Timoshkov wrote:
> "Jeff L"  wrote:
> > +DWORD WineEngGetGlyphIndices(HDC hdc, GdiFont font, LPCWSTR lpstr, 
> > INT count,
> >  LPWORD pgi, DWORD flags)
> > {
> >     INT i;
> > -
> > - TEXTMETRICW textm;
> > for(i = 0; i < count; i++)
> > - {
> >    pgi[i] = get_glyph_index(font, lpstr[i]);
> > -
> > -
> >    if  (pgi[i] == 0)
> > -
> >    {
> > -
> >        if  (flags & GGI_MARK_NONEXISTING_GLYPHS)
> > -
> >            pgi[i] = 0x001f;                  /* Indicate non 
> >
> > existance */
> > -
> >        else
> > -
> >        {
> > -
> >            GetTextMetricsW(hdc, &textm);
> > -
> >            pgi[i] = textm.tmDefaultChar;
> > -
> >        }
> > -
> >    }
> > - }
> > return count;
> >
> > }
>
> I'd suggest to move GetTextMetricsW outside of the loop to not kill
> the performance.

I put it inside the loop as I assumed that  a non existent  glyph would 
be relatively rare and the  call would not happen much.  This seemed 
preferable to doing the call every time the function was called.
> Please set tab width to 8 instead of 4 and do not mix tabs and spaces.

These slip through occasionally when using MS Studio as an editor for 
developing tests.  A bad habit but in general productive.

Jeff

resend add missing glyph code to GetGlyphIndices

[Andreas Mohr]

Hi,

On Mon, Aug 14, 2006 at 06:49:00PM +1000, Jeff Latimer wrote:
> Dmitry Timoshkov wrote:
> > I'd suggest to move GetTextMetricsW outside of the loop to not kill
> > the performance.
>
> I put it inside the loop as I assumed that  a non existent  glyph would 
> be relatively rare and the  call would not happen much.  This seemed 
> preferable to doing the call every time the function was called.

This should have gone into a comment right there because it's
a very normal reaction to immediately question code like that,
so the code should properly defend itself by default ;)

IOW just the usual "do coding as obvious as possible, then properly comment
everything else that isn't obvious".

Maybe something like
/* called in a loop, but missing glyph shouldn't happen often
   so we don't want to call it outside the loop, always */

Andreas Mohr

resend add missing glyph code to GetGlyphIndices

[Jeff Latimer]

Andreas Mohr wrote:
> This should have gone into a comment right there because it's
> a very normal reaction to immediately question code like that,
> so the code should properly defend itself by default ;)
>
> IOW just the usual "do coding as obvious as possible, then properly comment
> everything else that isn't obvious".
>
> Maybe something like
> /* called in a loop, but missing glyph shouldn't happen often
>   so we don't want to call it outside the loop, always */
>
> Andreas Mohr

Makes sense now that you mention it.

Jeff Latimer