2009/7/19 Matteo Bruni <matteo.myst...@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.