D3D shader assembler

[Matteo Bruni]

2009/7/19 Henri Verbeet <hverbeet at gmail.com>:
> 2009/7/19 Matteo Bruni <matteo.mystral at gmail.com>:
>> Hi to everybody,
>> I'm sending here the main assembler patch for reviews and suggestions.
>> It is bzipped as it is quite a big patch, but I couldn't find a
>> meaningful way to split it.
> It really is much easier to review as separate patches. The part that
> follows is really only the first thing I noticed.
>
>> +const char *debug_src(struct shader_reg *reg, BOOL vs) {
>> +    static char buffer[128];
>> +
>> +    memset(buffer, 0, sizeof(buffer));
> This really isn't acceptable:
>  - "debug_src" isn't a very good name for something that visible
> outside the current file
>  - The reg parameter should be const.
>  - Zeroing the entire buffer is quite unnecessary.
>  - Just passing the shader type would be much better than a BOOL
> parameter "vs" and hoping that anything that isn't a vertex shader is
> a pixel shader. Even if it happens to be true.
>  - "buffer" has a fixed size.
>  - "buffer" is static. Using wine_dbg_sprintf() would already be much
> better, but the entire "parsed_shader" setup looks flawed to me. It
> seems to me that you should handle that by having an appropriately
> filled struct asmparser_backend, and appending to a proper buffer.
>
> Note that I think your mentor should have caught basic things like
> this in a much earlier stage.
>

That function, in particular, should really be into asmparser.c and
not be visible from outside. Then the wine_dbg_sprintf() function
comes really handy in this situation, I didn't know it. Note also that
this debug_src function is used just to print trace info during asm
parsing, not to generate the intermediate representation or the
bytecode.
However, I think I've got your point. I'm going to look to other
similar mistakes in the code before resending.
A question: do you have an idea how I could split this in separate
patches? I can think of separating the parser from the bytecode
writer, but doesn't seems to me that this would be a real improvement.
Adding some shader instructions handling each time (for ex. starting
with just shader model 1, then separate patches for the subsequent
versions) maybe is better, but the first patch won't be really simpler
than this, I believe, as it would be alike this patch but without a
bunch of cases/functions.

Thank you,
Matteo.