I only looked at the first patch. 2009/8/9 Matteo Bruni <matteo.mystral(a)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.