Re: [PATCH v2] d3d10: Implement ID3D10EffectScalarVariable::{Get, Set}{Int, Float, Bool}

[Henri Verbeet]

Hi Nick,

Thank you for the patch. I do have a few comments though:

On Mon, 11 Mar 2019 at 19:40, Nick Renieris  wrote:
> +struct d3d10_effect_scalar_variable
> +{
> - union
> - {
> -
>    int i;
> -
>    float f;
> -
>    BOOL b;
> - };
>
> +};

Please avoid nameless unions.
> static HRESULT STDMETHODCALLTYPE d3d10_effect_scalar_variable_SetFloat(ID3D10EffectScalarVariable *iface,
>          float value)
>  {
> - FIXME("iface %p, value %.8e stub!\n", iface, value);
> - struct d3d10_effect_variable *This;

I know a bunch of other code in the same file also does this, but
please don't name variables "This". This may be one of those rare
cases where "variable" would be a perfectly reasonable name, or
perhaps simply "v".
> - if (!iface)
> -
>    return D3DERR_INVALIDCALL;

This check shouldn't be needed.

It would be great if you could write some tests to go along with this
patch. Also, this patch implements 6 different functions; please split
it so that each patch only makes a single conceptual change.