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

Józef Kucia joseph.kucia at gmail.com
Wed Mar 13 03:15:19 CDT 2019


On Tue, Mar 12, 2019 at 11:02 PM Nick Renieris <velocityra at gmail.com> wrote:
> > 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?

"This" is used in the old code. We don't use "This" anymore in D3D code.

> > 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?

I think that 6 commits are preferred. It should result in a cleaner
commit messages, i. e. "Implement
ID3D10EffectScalarVariable::GetInt()."

Also, tests would be useful. Your current implementation might be
incorrect. Is it legal to call SetFloat() on boolean variables?



More information about the wine-devel mailing list