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 <velocityra(a)gmail.com> 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.