2009/8/10 Stefan Dösinger <stefandoesinger(a)gmx.at>:
> Hi,
> A few comments - mostly things I haven't spotted earlier.
Hi,
I fixed the code, almost completely following your reviews (thank you,
of course).
I'm reporting the differences with your suggestions:
>
>> --- /dev/null
>> +++ b/dlls/d3dx9_36/asmshader.h
>> --- /dev/null
>> +++ b/dlls/d3dx9_36/asmshader_private.h
> I think it doesn't need two include files, especially since both of them are
> d3dx9-private anyway. The question is, is it better to just merge them into
> d3dx9_36_private.h, or have assembler things in a separate file? I tend
> towards putting everything into d3dx9_36_private.h, but I don't have a strong
> opinion about that.
>
I merged everything into d3dx9_36_private.h
>> +%option prefix="asmshader_"
>> + buffer = asmshader__scan_string(text, asm_ctx.yyscanner);
> It seems that flex adds an underscore to the prefix anyway, so I guess its
> fine to remove the one from the prefix option.
>
Removing the underscore from the flex prefix produces a (better)
asmshader_scan_string function but also an ugly asmshaderlex_init. I
decided to keep the underscore.
>> + WINED3DSIO_NOP = 0,
>> + WINED3DSIO_MOV = 1,
>> + WINED3DSIO_ADD = 2,
>> and others
> Can you give them a different name prefix, to make it clear that they don't
> have any relationship with wined3d any more?
>
I named them BWRITERxxxx (the same for wine_shader, now it is named
bwriter_shader, enforcing the idea that that represents the bytecode
writer interface). Is it ok?
>>+DWORD SlWriteBytecode(const wine_shader *shader, int dxversion, DWORD
> **result) {
>> +static struct bc_writer *create_writer(DWORD version, DWORD dxversion) {
> Does it still need the dxversion?
>
It's not needed, but I kept it in anticipation of D3D10 support. Can
be removed anyway.
> asmparser_srcreg_vs_3:
>> + if(src->type != WINED3DSPR_TEMP && src->type != WINED3DSPR_INPUT &&
>>+ src->type != WINED3DSPR_CONST && src->type != WINED3DSPR_ADDR &&
>> + src->type != WINED3DSPR_CONSTBOOL && src->type !=
> WINED3DSPR_CONSTINT &&
>> + src->type != WINED3DSPR_LOOP && src->type !=WINED3DSPR_SAMPLER &&
>> + src->type != WINED3DSPR_LABEL && src->type != WINED3DSPR_PREDICATE)
> A swich-case statement or a lookup table would probably be nicer. (A lookup
> table has the disadvantage that you still have to check min and max
> separately though)
Looking better at this, I noticed that I wasn't checking the register
number, while native has this check. So I reworked this part, and now
the code is as the attached file.
Regarding Henri's review, I followed everything, except keeping
my_alloc/realloc/free (now asm_alloc and c.) functions, as it seems
Stefan likes them very much :)
Supposing my changes are ok, what should I do now? Wait for reviews of
the rest? Or maybe send the updated code? In this case, should I send
it again in wine-devel or have a try with wine-patches?
Cheers,
Matteo Bruni