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