Re: [1/18] d3dx9: Add shader assembler private definitions
Don't send patches that only add unused code. On Sun, Aug 16, 2009 at 12:50 PM, Matteo Bruni<matteo.mystral(a)gmail.com> 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.
-- Vincent Povirk
2009/8/16 Vincent Povirk <madewokherd+8cd9(a)gmail.com>:
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.
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
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.
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
2009/8/18 Matteo Bruni <matteo.mystral(a)gmail.com>:
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.
2009/8/18 Henri Verbeet <hverbeet(a)gmail.com>:
2009/8/18 Matteo Bruni <matteo.mystral(a)gmail.com>:
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...
participants (4)
-
Henri Verbeet -
Juan Lang -
Matteo Bruni -
Vincent Povirk