D3D shader assembler (round 2)

[Stefan Dösinger]

Hi,
A few comments - mostly things I haven't spotted earlier.

> --- /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.

> +%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.

> +       | VER_DX8_VS11
> +        {
> +                TRACE("Vertex shader 1.1 dx8\n");
> +                   /* TODO: create the appropriate parsercontext*/
> +        }
Is that still needed? I know that the "vs.1.1" start token is still valid in 
d3dx9, but afair it behaves like vs_1_1, so this can be handed in the lexer 
rule

> +  Here below there are some enumerations and defines previously included
> +  from wined3d_private_types.h
My long term experience is that after a certain period of time comments where 
the code came from(as opposed to why it was written that way, and not some 
other way) aren't helpful any more. Probably just remove the comment, and 
also see the point below:

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

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

>+    unsigned int line_no;
> ...
> +        asmparser_message(This, "Line %d: Source modifier %s not supported 
in this shader version\n",
> +                          This->line_no,
> +                          debug_print_srcmod(srcmod));
Please use %u for unsigned ints(happens in quite a few places)

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)

Still looking at the remaining code, so more might follow if I find more.