Re: [PATCH 1/2] d3drm: Change signature of d3drm_texture_create.

[Henri Verbeet]

The subject line is pretty vague.

On 15 March 2016 at 22:19, Aaryaman Vasishta  wrote:
> @@ -193,8 +194,16 @@ static HRESULT WINAPI d3drm1_CreateTexture(IDirect3DRM *iface,
>          D3DRMIMAGE *image, IDirect3DRMTexture **texture)
>  {
>      FIXME("iface %p, image %p, texture %p partial stub.\n", iface, image, texture);
> - struct d3drm_texture *object;
> - HRESULT hr;

You mix declarations and code here. The compiler should have warned about that.
> - hr = d3drm_texture_create(&object);
> - if (FAILED(hr))
> -
>    return hr;

We would generally write that as
    if (FAILED(hr = d3drm_texture_create(&object)))
        return hr;
not a big deal though.
> +IDirect3DRMTexture *IDirect3DRMTexture_from_impl(struct d3drm_texture *texture)
> +{
> - return &texture->IDirect3DRMTexture_iface;
>
> +}
> -
>
> +IDirect3DRMTexture2 *IDirect3DRMTexture2_from_impl(struct d3drm_texture *texture)
> +{
> - return &texture->IDirect3DRMTexture2_iface;
>
> +}
> -
>
> +IDirect3DRMTexture3 *IDirect3DRMTexture3_from_impl(struct d3drm_texture *texture)
> +{
> - return &texture->IDirect3DRMTexture3_iface;
>
> +}

I don't think these are an improvement, I'd prefer them just written out.
> -HRESULT Direct3DRMTexture_create(REFIID riid, IUnknown **out)
> +HRESULT d3drm_texture_create(struct d3drm_texture **out)

I think "texture" would be a more appropriate name than "out" here.

[PATCH 1/2] d3drm: Change signature of d3drm_texture_create.

[Aaryaman Vasishta]

On Wed, Mar 16, 2016 at 5:29 PM, Henri Verbeet  wrote:
> >  FIXME("iface %p, image %p, texture %p partial stub.\n", iface,
>
> image, texture);
> > - struct d3drm_texture *object;
> > - HRESULT hr;
>
> You mix declarations and code here. The compiler should have warned about
> that.

clang didn't warn me about that. I should still fix it though. Does clang
need -Wall for that warning to come?
> > +IDirect3DRMTexture *IDirect3DRMTexture_from_impl(struct d3drm_texture
>
> *texture)
> > +{
> > - return &texture->IDirect3DRMTexture_iface;
> >
> > +}
> > -
> >
> > +IDirect3DRMTexture2 *IDirect3DRMTexture2_from_impl(struct d3drm_texture
>
> *texture)
> > +{
> > - return &texture->IDirect3DRMTexture2_iface;
> >
> > +}
> > -
> >
> > +IDirect3DRMTexture3 *IDirect3DRMTexture3_from_impl(struct d3drm_texture
>
> *texture)
> > +{
> > - return &texture->IDirect3DRMTexture3_iface;
> >
> > +}
>
> I don't think these are an improvement, I'd prefer them just written out.

That's fine with me too, I kept them there for future use, but that doesn't
seem to be the case. I should add these only if really required (like for
devices).

I'm currently caught up in a college project presentation which ends this
weekend, I'll submit all changes this weekend mostly.

Thanks for the review! Once this is done we can move on to the 2nd patch
(of which i'll be sending another patch which will apply without the
trailing whitespace, though more revision is required there anyways.)

Cheers,
Aaryaman

[PATCH 1/2] d3drm: Change signature of d3drm_texture_create.

[Henri Verbeet]

On 16 March 2016 at 21:34, Aaryaman Vasishta  wrote:
> clang didn't warn me about that. I should still fix it though. Does clang
> need -Wall for that warning to come?

-Wdeclaration-after-statement will probably work, but I'd be inclined
to say "Just use gcc".
> > > +IDirect3DRMTexture *IDirect3DRMTexture_from_impl(struct d3drm_texture
> > > *texture)
> > > +{
> > > - return &texture->IDirect3DRMTexture_iface;
> > >
> > > +}
> > > -
> > >
> > > +IDirect3DRMTexture2 *IDirect3DRMTexture2_from_impl(struct d3drm_texture
> > > *texture)
> > > +{
> > > - return &texture->IDirect3DRMTexture2_iface;
> > >
> > > +}
> > > -
> > >
> > > +IDirect3DRMTexture3 *IDirect3DRMTexture3_from_impl(struct d3drm_texture
> > > *texture)
> > > +{
> > > - return &texture->IDirect3DRMTexture3_iface;
> > >
> > > +}
> >
> > I don't think these are an improvement, I'd prefer them just written out.
>
> That's fine with me too, I kept them there for future use, but that doesn't
> seem to be the case. I should add these only if really required (like for
> devices).

They're not really required there either. It's nice to keep the struct
d3drm_device definition local to device.c, but not at all costs. And
I'd expect it to get harder and harder to keep it there once you start
actually implementing things.