[PATCH 8/9] gdi32: Use META_EXTFLOODFILL records for ExtFloodFill.

[Jacek Caban]

Signed-off-by: Jacek Caban 
---
  dlls/gdi32/mfdrv/graphics.c    |  4 +--
  dlls/gdi32/mfdrv/init.c        | 17 ++++++++++++
  dlls/gdi32/mfdrv/metafiledrv.h |  2 ++
  dlls/gdi32/tests/metafile.c    | 48 +++++++++++++++++++++++++++++++++-
  4 files changed, 68 insertions(+), 3 deletions(-)

[Dmitry Timoshkov]

Jacek Caban  wrote:
> +BOOL metadc_param5( HDC hdc, short func, short param1, short param2,
> -
>                short param3, short param4, short param5 )
>
> +{
> - char buffer[16];
> - METARECORD *mr = (METARECORD *)&buffer;
> - WORD *params = mr->rdParm;
> -
> - mr->rdSize = 8;
> - mr->rdFunction = func;
> - params[0] = param5;
> - params[1] = param4;
> - params[2] = param3;
> - params[3] = param2;
> - params[4] = param1;
> - return metadc_record( hdc, mr, mr->rdSize * 2);
>
> +}

sizeof(METARECORD) == 4 + 2 + 2 = 8 bytes, 5 * sizeof(params[0]) = 10 bytes,
and buffer[16] won't hold all of these. Wouldn't it be better to use something
like sizeof(FIELD_OFFSET(METARECORD, rdParm[5])) instead of hardcoded 16?

[Jacek Caban]

On 7/25/21 11:18 AM, Dmitry Timoshkov wrote:
> Jacek Caban  wrote:
> > +BOOL metadc_param5( HDC hdc, short func, short param1, short param2,
> > -
> >                short param3, short param4, short param5 )
> >
> > +{
> > - char buffer[16];
> > - METARECORD *mr = (METARECORD *)&buffer;
> > - WORD *params = mr->rdParm;
> > -
> > - mr->rdSize = 8;
> > - mr->rdFunction = func;
> > - params[0] = param5;
> > - params[1] = param4;
> > - params[2] = param3;
> > - params[3] = param2;
> > - params[4] = param1;
> > - return metadc_record( hdc, mr, mr->rdSize * 2);
> >
> > +}
>
> sizeof(METARECORD) == 4 + 2 + 2 = 8 bytes, 5 * sizeof(params[0]) = 10 bytes,
> and buffer[16] won't hold all of these. Wouldn't it be better to use something
> like sizeof(FIELD_OFFSET(METARECORD, rdParm[5])) instead of hardcoded 16?

You counted rdParam[0] twice, once as part of sizeof(METARECORD) and 
once as a param[0] size. The size if fine AFAICT.

That said, I agree that there is some room for improvements. I simply 
copied the code from MFDRV_MetaParam6 and adjusted it. I was planning to 
revisit that when the conversion is more complete. Once all metafile 
recording is independent of driver interface, we may inline 
MFDRV_MetaParam* functions into metadc_* functions, taking HDC instead 
of PHYSDEV as an argument. I may as well do that now for new functions...

Thanks,

Jacek

[Dmitry Timoshkov]

Jacek Caban  wrote:
> On 7/25/21 11:18 AM, Dmitry Timoshkov wrote:
> > Jacek Caban  wrote:
> > > +BOOL metadc_param5( HDC hdc, short func, short param1, short param2,
> > > -
> > >                short param3, short param4, short param5 )
> > >
> > > +{
> > > - char buffer[16];
> > > - METARECORD *mr = (METARECORD *)&buffer;
> > > - WORD *params = mr->rdParm;
> > > -
> > > - mr->rdSize = 8;
> > > - mr->rdFunction = func;
> > > - params[0] = param5;
> > > - params[1] = param4;
> > > - params[2] = param3;
> > > - params[3] = param2;
> > > - params[4] = param1;
> > > - return metadc_record( hdc, mr, mr->rdSize * 2);
> > >
> > > +}
> >
> > sizeof(METARECORD) == 4 + 2 + 2 = 8 bytes, 5 * sizeof(params[0]) = 10 bytes,
> > and buffer[16] won't hold all of these. Wouldn't it be better to use something
> > like sizeof(FIELD_OFFSET(METARECORD, rdParm[5])) instead of hardcoded 16?
>
> You counted rdParam[0] twice, once as part of sizeof(METARECORD) and 
> once as a param[0] size. The size if fine AFAICT.

You're right, sorry for the sloppy comment.
> That said, I agree that there is some room for improvements. I simply 
> copied the code from MFDRV_MetaParam6 and adjusted it. I was planning to 
> revisit that when the conversion is more complete. Once all metafile 
> recording is independent of driver interface, we may inline 
> MFDRV_MetaParam* functions into metadc_* functions, taking HDC instead 
> of PHYSDEV as an argument. I may as well do that now for new functions...