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

[Nick Renieris]

Hi,

Στις Τρί, 12 Μαρ 2019 στις 1:25 μ.μ., ο/η Henri Verbeet
 έγραψε:
> 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.

Ok, I'll make `d3d10_effect_scalar_variable` a union.
> > 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".

"This" is indeed used everywhere. Can you explain what's different in my case?
> 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.

I have no issue with doing that, but is it _really_ necessary for
something so simple?
If so, can I split to 3 commits, grouping Set/Get functions together?

Thanks,
Nick.