Re: [2/2] gdi32: implementation of CreateFontIndirectEx (fix for13064) (resent)

[Dmitry Timoshkov]

"Nikolay Sivov"  wrote:
> +/***********************************************************************
> -
> -
>       CreateFontIndirectExA   (GDI32.@)
> - */
>
> +HFONT WINAPI CreateFontIndirectExA( const ENUMLOGFONTEXDVA *plf )
> +{
> - if(!plf)  return NULL;
> -
> - return CreateFontIndirectA(&(plf->elfEnumLogfontEx.elfLogFont));
>
> +}
> -
>
> +/***********************************************************************
> -
> -
>       CreateFontIndirectExW   (GDI32.@)
> - */
>
> +HFONT WINAPI CreateFontIndirectExW( const ENUMLOGFONTEXDVW *plf )
> +{
> - if(!plf)  return NULL;
> -
> - return CreateFontIndirectW(&(plf->elfEnumLogfontEx.elfLogFont));
>
> +}

You neither removed the 'if(!plf)' check, nor added a test or an explanation
for this.
> - if(!pCreateFontIndirectExA)
> -
>    skip("CreateFontIndirectExA not implemeted on this platform\n");
> - else
> - {
> -
>    memcpy(&lfex.elfEnumLogfontEx.elfLogFont, &lf, sizeof(lf));
> -
>    hfont = create_font_ex("Arial", &lfex);
> -
>    DeleteObject(hfont);
> - }

Presumably CreateFontIndirectEx uses the whole ENUMLOGFONTEXDVA structure,
not just LOGFONT, so you need to fully initialize it.

Also please remove trailing line spaces.

[2/2] gdi32: implementation of CreateFontIndirectEx (fix for13064) (resent)

[Nikolay Sivov]

Dmitry Timoshkov wrote:
> "Nikolay Sivov"  wrote:
> > +/***********************************************************************
> > -
> > -
> >       CreateFontIndirectExA   (GDI32.@)
> > - */
> >
> > +HFONT WINAPI CreateFontIndirectExA( const ENUMLOGFONTEXDVA *plf )
> > +{
> > - if(!plf)  return NULL;
> > -
> > - return CreateFontIndirectA(&(plf->elfEnumLogfontEx.elfLogFont));
> >
> > +}
> > -
> >
> > +/***********************************************************************
> > -
> > -
> >       CreateFontIndirectExW   (GDI32.@)
> > - */
> >
> > +HFONT WINAPI CreateFontIndirectExW( const ENUMLOGFONTEXDVW *plf )
> > +{
> > - if(!plf)  return NULL;
> > -
> > - return CreateFontIndirectW(&(plf->elfEnumLogfontEx.elfLogFont));
> >
> > +}
>
> You neither removed the 'if(!plf)' check, nor added a test or an 
> explanation
> for this.

Here I don't follow you. If I want to access elfEnumLogfontEx field 
through pointer,
so should I check this pointer first? I thought it's obvious or maybe 
there're different things
we are talking about?
Could you explain please..
> > - if(!pCreateFontIndirectExA)
> > -
> >    skip("CreateFontIndirectExA not implemeted on this 
> >
> > platform\n");
> > - else
> > - {
> > -
> >    memcpy(&lfex.elfEnumLogfontEx.elfLogFont, &lf, sizeof(lf));
> > -
> >    hfont = create_font_ex("Arial", &lfex);
> > -
> >    DeleteObject(hfont);
> > - }
>
> Presumably CreateFontIndirectEx uses the whole ENUMLOGFONTEXDVA 
> structure,
> not just LOGFONT, so you need to fully initialize it.
>
> Also please remove trailing line spaces.

Here I'm fully agreed. Full initialization should be made, I thought 
only about LOGFONT part..

[2/2] gdi32: implementation of CreateFontIndirectEx (fix for13064)(resent)

[Dmitry Timoshkov]

"Nikolay Sivov"  wrote:
> > You neither removed the 'if(!plf)' check, nor added a test or an 
> > explanation
> > for this.
>
> Here I don't follow you. If I want to access elfEnumLogfontEx field 
> through pointer,
> so should I check this pointer first? I thought it's obvious or maybe 
> there're different things
> we are talking about?
> Could you explain please..

If Windows doesn't check for plf being NULL and crashes Wine should crash
as well. If Windows doesn't crash in that case the test for this behaviour
should be added.

[2/2] gdi32: implementation of CreateFontIndirectEx (fix for13064)(resent)

[Nikolay Sivov]

Dmitry Timoshkov wrote:
> "Nikolay Sivov"  wrote:
> > > You neither removed the 'if(!plf)' check, nor added a test or an 
> > > explanation
> > > for this.
> >
> > Here I don't follow you. If I want to access elfEnumLogfontEx field 
> > through pointer,
> > so should I check this pointer first? I thought it's obvious or maybe 
> > there're different things
> > we are talking about?
> > Could you explain please..
>
> If Windows doesn't check for plf being NULL and crashes Wine should crash
> as well. If Windows doesn't crash in that case the test for this 
> behaviour
> should be added.

Ok, I got it. Windows doesn't crash on NULL, so a test will be added.

Next question is when I'm trying to initialize other fields of 
ENUMLOGFONTEXA I see some compiler warnings
about "differ in signedness". It means that already defined ENUMLOGFONT 
and ENUMLOGFONTEX
(in wingdi.h) use BYTE as type for elfFullName[LF_FULLFACESIZE] etc. 
Could I change it to CHAR to eliminate
this warning? Could it break something?

[2/2] gdi32: implementation of CreateFontIndirectEx (fixfor13064)(resent)

[Dmitry Timoshkov]

"Nikolay Sivov"  wrote:
> Next question is when I'm trying to initialize other fields of 
> ENUMLOGFONTEXA I see some compiler warnings
> about "differ in signedness". It means that already defined ENUMLOGFONT 
> and ENUMLOGFONTEX
> (in wingdi.h) use BYTE as type for elfFullName[LF_FULLFACESIZE] etc. 
> Could I change it to CHAR to eliminate
> this warning? Could it break something?

Wine should use exactly the same types as Windows SDK does.