2009/7/19 Henri Verbeet hverbeet@gmail.com:
2009/7/19 Matteo Bruni matteo.mystral@gmail.com:
Hi to everybody, I'm sending here the main assembler patch for reviews and suggestions. It is bzipped as it is quite a big patch, but I couldn't find a meaningful way to split it.
It really is much easier to review as separate patches. The part that follows is really only the first thing I noticed.
+const char *debug_src(struct shader_reg *reg, BOOL vs) {
- static char buffer[128];
- memset(buffer, 0, sizeof(buffer));
This really isn't acceptable: - "debug_src" isn't a very good name for something that visible outside the current file - The reg parameter should be const. - Zeroing the entire buffer is quite unnecessary. - Just passing the shader type would be much better than a BOOL parameter "vs" and hoping that anything that isn't a vertex shader is a pixel shader. Even if it happens to be true. - "buffer" has a fixed size. - "buffer" is static. Using wine_dbg_sprintf() would already be much better, but the entire "parsed_shader" setup looks flawed to me. It seems to me that you should handle that by having an appropriately filled struct asmparser_backend, and appending to a proper buffer.
Note that I think your mentor should have caught basic things like this in a much earlier stage.
That function, in particular, should really be into asmparser.c and not be visible from outside. Then the wine_dbg_sprintf() function comes really handy in this situation, I didn't know it. Note also that this debug_src function is used just to print trace info during asm parsing, not to generate the intermediate representation or the bytecode. However, I think I've got your point. I'm going to look to other similar mistakes in the code before resending. A question: do you have an idea how I could split this in separate patches? I can think of separating the parser from the bytecode writer, but doesn't seems to me that this would be a real improvement. Adding some shader instructions handling each time (for ex. starting with just shader model 1, then separate patches for the subsequent versions) maybe is better, but the first patch won't be really simpler than this, I believe, as it would be alike this patch but without a bunch of cases/functions.
Thank you, Matteo.