D3D shader assembler (round 2)

[Henri Verbeet]

I only looked at the first patch.

2009/8/9 Matteo Bruni <matteo.mystral at gmail.com>:
> +static void asmparser_instr(struct asm_parser *This, DWORD opcode,
...
> +    if(srcs) {
> +        for(i=0; i<srcs->nr; i++) {
...
> +    if(srcs && (srcs->nr != expectednsrcs)) {
...
> +    if(srcs) {
> +        for(i=0; i<srcs->nr; i++) {
You can simplify that a bit by doing something like "unsigned int
src_count = srcs ? src->nr : 0;" at the start, and then just using
src_count.

> +    rc = vsnprintf(base, MESSAGEBUFFER_SIZE - 1 - ctx->messagesize, fmt, args);
You should include wine/port.h if you use vsnprintf(). It's probably
not a bad idea to just include that in general.

> +typedef struct wine_shader_ {
That's not very nice (both the name and the typedef).

> +static inline LPVOID my_alloc(SIZE_T size) {
You'll want to give that a better name than "my_alloc", although I
wonder if you need a function for this at all.

> +struct src_regs {
> +    struct shader_reg reg[MAX_SRC_REGS];
> +    int               nr;
> +};
I think "nr" can be unsigned. Personally I'd named the variable "count".

> +#define MESSAGEBUFFER_SIZE 4096
> +struct asm_parser {
> +    /* The function table of the parser implementation */
> +    struct asmparser_backend *funcs;
> +
> +    /* Private data follows */
> +    wine_shader              *shader;
> +    unsigned int              m3x3pad_count;
> +
> +    /* Reentrant lexer pointer */
> +    void *yyscanner;
> +
> +    enum parse_status{
> +        PARSE_SUCCESS = 0,
> +        PARSE_WARN = 1,
> +        PARSE_ERR = 2
> +    } status;
> +    char messages[MESSAGEBUFFER_SIZE];
> +    unsigned int messagesize;
> +    unsigned int line_no;
> +};
A dynamically allocated message buffer wouold probably be better.

> +#define SWIZZLE_ERR -1
...
> +                return SWIZZLE_ERR; /* 0 is a valid swizzle */
DWORDs are unsigned, so you should define SWIZZLE_ERR to something
like ~0U. Also, if you allow text2swizzle() to return an error, you
should check for it when calling the function.

> +void add_instruction(wine_shader *shader, struct instruction *instr) {
> +    struct instruction      **new_instructions;
> +
> +    if(shader->instr_alloc_size == 0) {
> +        shader->instr = my_alloc(sizeof(*shader->instr));
> +        shader->instr_alloc_size = 1;
> +    } else if(shader->instr_alloc_size == shader->num_instrs) {
> +        shader->instr_alloc_size += max(shader->instr_alloc_size, 1);
> +        new_instructions = my_realloc(shader->instr,
> +                                      sizeof(*shader->instr) * (shader->instr_alloc_size));
> +        if(!new_instructions) {
> +            ERR("Failed to grow the shader instruction array\n");
> +            return;
> +        }
> +        shader->instr = new_instructions;
> +    } else if(shader->num_instrs > shader->instr_alloc_size) {
> +        ERR("More instructions than allocated. This should not happen\n");
> +    }
> +
> +    shader->instr[shader->num_instrs] = instr;
> +    shader->num_instrs++;
> +}
This function can fail, so the caller needs to handle the failure.

> +    if(shader->num_cf) {
> +        shader->constF = my_realloc(shader->constF,
> +                                    sizeof(*shader->constF) * (shader->num_cf + 1));
> +    } else {
> +        shader->constF = my_alloc(sizeof(*shader->constF));
> +    }
> +
> +    newconst = my_alloc(sizeof(*newconst));
These can fail. Some of the other functions have similar flaws.

> +        shader->samplers = my_realloc(shader->samplers,
> +                                      sizeof(*shader->samplers) * (shader->num_samplers + 1));
> +    }
> +    if(!shader->samplers) {
> +        ERR("Out of memory\n");
> +        return FALSE;
> +    }
If my_realloc() fails, you've now leaked the original shader->samplers memory.

> +    /* Allocate 8 DWORDs for the start - even a minimalistic shader will need
> +     * some instructions. Allocating it here makes the growing code easier since
> +     * it just needs a realloc then
> +     */
> +    ret->alloc_size = 8;
I think you should aim for an "average" shader rather than a minimal one here.

> +        buffer->data = my_realloc(buffer->data,
> +                                  sizeof(DWORD) * buffer->alloc_size);
> +        if(!buffer->data) {
> +            ERR("Failed to grow the buffer data memory\n");
> +            buffer->state = E_OUTOFMEMORY;
> +        }
Similar to record_sampler() above, you'll leak the original
buffer->data memory if my_realloc() fails.