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.