Re: [PATCH 4/5] ddraw: Accept all 3 versions of D3DFINDDEVICERESULT

[Andrew D'Addesio]

On 10/05/2015 03:57 AM, Henri Verbeet wrote:
> On 5 October 2015 at 09:21, Stefan Dösinger  wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA256
> >
> > Am 2015-10-04 um 19:52 schrieb Andrew D'Addesio:
> > > - /* Write ddHwDesc */
> > > - desc_ptr = (BYTE*)&fdr->ddHwDesc;
> > > - memcpy(desc_ptr, &desc1, desc_size);
> > > - ((D3DDEVICEDESC*)desc_ptr)->dwSize = desc_size;
> > > -
> > > - /* Write ddSwDesc */
> > > - desc_ptr += desc_size;
> > > - memcpy(desc_ptr, &desc1, desc_size);
> > > - ((D3DDEVICEDESC*)desc_ptr)->dwSize = desc_size;
> >
> > Is there a nicer way? I can't think of one to be honest, but this code
> > feels somewhat unsatisfying.
>
> One way would be to just declare different versions of the structure
> for internal use (i.e., D3DFINDDEVICERESULTv1, etc.), and then pick
> the right one based on fdr->dwSize. That would probably simplify a
> couple of other things in the patch as well.

In that case, we could do:
> if (ver == 1)
> {
>     D3DFINDDEVICERESULTv1 *fdrv1 = (D3DFINDDEVICERESULTv1 *)fdr;
>     D3DDEVICEDESCv1 *desc1v1 = (D3DDEVICEDESCv1 *)&desc1;
>     fdrv1->ddHwDesc = *desc1v1;
>     fdrv1->ddSwDesc = *desc1v1;
> }
> else if (ver == 2)
> {
>     /* ... */

On 10/05/2015 03:57 AM, Henri Verbeet wrote:
> There are also a couple
> of style issues in these patches. Please follow the style of the
> surrounding code, or that of wined3d when in doubt.

Sorry, I was so busy checking for code correctness that I forgot about
indentation. I think that's mainly what you're referring to. For example, this:
>        hel_desc.dwFlags = D3DDD_COLORMODEL | D3DDD_DEVCAPS | D3DDD_TRANSFORMCAPS
>            | D3DDD_LIGHTINGCAPS | D3DDD_BCLIPPING;

should become:
>        hel_desc.dwFlags = D3DDD_COLORMODEL | D3DDD_DEVCAPS | D3DDD_TRANSFORMCAPS
>                | D3DDD_LIGHTINGCAPS | D3DDD_BCLIPPING;

and similarly:
>    ok(device > ctx->prev_device, "D3D%d %s enumerated after %s\n", ver,
>        expected_name, device_list[ctx->prev_device].device_name);

should become:
>    ok(device > ctx->prev_device, "D3D%d %s enumerated after %s\n", ver,
>       expected_name, device_list[ctx->prev_device].device_name);

And I think the other big problem was that I did "enum {" instead of placing
the { on its own line.

Is there anything else you had in mind that I should know about?

Thanks,
Andrew