Re: [1/18] d3dx9: Add shader assembler private definitions

[Vincent Povirk]

Don't send patches that only add unused code.

On Sun, Aug 16, 2009 at 12:50 PM, Matteo Bruni wrote:
> This is a long patch series that implements the D3D shader assembler
> and D3DXAssembleShader function. The first part has already passed a
> couple of rounds of review in wine-devel.

[1/18] d3dx9: Add shader assembler private definitions

[Matteo Bruni]

2009/8/16 Vincent Povirk :
> Don't send patches that only add unused code.

Well, to be true all the first 12 patches add code which is
effectively used only from patch 13 onwards. But I couldn't find a
better way to split the patches in manageable pieces while avoiding
these kind of issues.

[1/18] d3dx9: Add shader assembler private definitions

[Juan Lang]

> Well, to be true all the first 12 patches add code which is
> effectively used only from patch 13 onwards. But I couldn't find a
> better way to split the patches in manageable pieces while avoiding
> these kind of issues.

I tend to agree with Vincent here:  adding a bunch of dead code that
all gets enabled at once can make it rather hard to pinpoint the
source of regressions.  Your patch numbering is wrong, as you know, as
your patch 4 belongs between patches 1 and 2, so that alone merits a
resend.  But I think the order is rather backwards anyway.  The
typical sequence is,

1) Change any public interfaces you need to.  I think your patch 14
falls into this category.
2) Add stubs for the public functions or interfaces you want to add.
For instance, you'd add stubs for D3DXAssembleShader,
D3DXAssembleShaderFromFile and D3DXAssembleShaderFromResource either
as one patch--often acceptable when adding stubs--or as separate
patches.
3) Add tests for the newly exposed functions or interfaces,
appropriately marked with todo_wine.
4) Incrementally add code to implement the functions or interfaces
you're implementing, removing todo_wine from any tests that begin to
pass as a result.

Step 4) really comprises many small patches, and will often comprise
the bulk of the patchset.  It's important to make each commit atomic
and useful.  So, for example, adding a bunch of declarations that
aren't used to a private header isn't useful, though it is atomic.
You'd only want to add to a private header declarations which are used
by code in the same patch.  So your patch 1 should be both split and
combined with other patches from your series.

I hope my general directions make some sense.

The difficulty with the patchset as it is is that it requires a great
deal of knowledge and a prodigious memory on the part of the reviewer.
 The reviewer must read and understand the entirety of your
implementation before he can begin to understand how it's used.  For
superhumans like Alexandre perhaps this is reasonable, but for mere
mortals like the rest of us, more easily digestible pieces are far
easier to understand.

Thanks,
--Juan

[1/18] d3dx9: Add shader assembler private definitions

[Matteo Bruni]

> Step 4) really comprises many small patches, and will often comprise
> the bulk of the patchset.  It's important to make each commit atomic
> and useful.  So, for example, adding a bunch of declarations that
> aren't used to a private header isn't useful, though it is atomic.

Hi Juan,
while in principle I agree with what you say, in practice it is
difficult for me to do this.
The assembler requires quite some code to be at least minimally
working (i.e. to implement a somewhat working D3DXAssembleShader
function). This means that the first patch of step 4 will be really
big. It will have as a starting point a merge of patch 1, 2, 3, 5, 6,
12, 15, which sums up to ~ 200KB. From there I will try to strip as
much as possible (pixel shader support, some shader language
constructs, single instructions, ...) but it seems to be quite a work
and in any case the resulting patch needs to have a working lexer,
parser, bytecode writer and the glue among them, so it can't be made
smaller than so much, I believe.
Anyway I'm going to try to rework the patches the way you suggest,
we'll see what comes up...

Thanks,
Matteo Bruni.

[1/18] d3dx9: Add shader assembler private definitions

[Juan Lang]

> The assembler requires quite some code to be at least minimally
> working (i.e. to implement a somewhat working D3DXAssembleShader
> function).

Is it not possible to implement enough to support some small subset of
the language?  If it's not at all possible, then we'll just have to
live with it.. but the odds of a favorable review get rather long.  So
if it would be possible to support the simplest shader program you can
think of with some subset of the code, that would be a worthwhile
approach, in my opinion.

Cheers,
--Juan

[1/18] d3dx9: Add shader assembler private definitions

[Henri Verbeet]

2009/8/18 Matteo Bruni :
> constructs, single instructions, ...) but it seems to be quite a work
> and in any case the resulting patch needs to have a working lexer,
> parser, bytecode writer and the glue among them, so it can't be made
> smaller than so much, I believe.

I don't think that's really a requirement. It's probably fine to e.g.
just add a minimal lexer first, then add a minimal parser, etc.

[1/18] d3dx9: Add shader assembler private definitions

[Matteo Bruni]

2009/8/18 Henri Verbeet :
> 2009/8/18 Matteo Bruni :
> > constructs, single instructions, ...) but it seems to be quite a work
> > and in any case the resulting patch needs to have a working lexer,
> > parser, bytecode writer and the glue among them, so it can't be made
> > smaller than so much, I believe.
>
> I don't think that's really a requirement. It's probably fine to e.g.
> just add a minimal lexer first, then add a minimal parser, etc.

Lexer and parser have to go together, as the parser defines the tokens
used by the lexer. In any case, a patch adding just the lexer is not
"useful" (as in Juan's definition) because it can't be used without
the other components. I believe in this case the choice is between
having a useful bigger patch or smaller patches but not useful by
themselves.
Maybe I'll have a better view of the situation after some tinkering
with the patches. Which probably will be tomorrow as now I my
development system is half-broken and I have to fix it...