Re: [1/3] d3dx9: Partial implementation of D3DXAssembleShader function. [try 2]
On 4 February 2010 19:26, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
+void wpp_write_message_var(const char *fmt, ...) PRINTF_ATTR(1,2); +void wpp_write_message_var(const char *fmt, ...) You can just do the following: +void PRINTF_ATTR(1,2) wpp_write_message_var(const char *fmt, ...)
+ desc = HeapAlloc(GetProcessHeap(), 0, sizeof(struct mem_file_desc)); sizeof(*desc) is generally easier, but that's minor of course.
+ if(desc->pos + len > desc->size) len = desc->size - desc->pos; It's usually safer to write expressions like that as "if (len > desc->size - desc->pos)" because "desc->pos + len" can wrap around if len is large, while "desc->size - desc->pos" should always be safe. Perhaps the caller is expected to always pass sane values here, but it's a somewhat dangerous construction in the general case in terms of reading/writing past the end of buffers. It can also be simplified to "len = min(len, desc->size - desc->pos);" here.
+ if(wpp_output_size + len > wpp_output_capacity) Similar issue as above.
+int wpp_close_output(void) +{ + /* trim buffer to the effective size */ + char *new_wpp_output = HeapReAlloc(GetProcessHeap(), 0, wpp_output, + wpp_output_size + 1); + if(!new_wpp_output) return 0; + wpp_output[wpp_output_size]='\0'; + return 1; +} This doesn't really make sense. The comment is misleading, because you actually grow the buffer if "wpp_output_size == wpp_output_capacity". If you didn't, you wouldn't care about HeapRealloc() failure because the worst thing that could happen was that the buffer was a bit larger than it strictly needed to be. More importantly though, you assume new_wpp_output is the same pointer as wpp_output after a successful HeapReAlloc(), which isn't necessarily true.
+ current_shader.buffer = HeapAlloc(GetProcessHeap(), 0, data_len + 1); ... + ret = wpp_parse("", NULL); + if(!wpp_close_output()) + ret = 1; + if(ret) + { + TRACE("Error during shader preprocessing\n"); + HeapFree(GetProcessHeap(), 0, current_shader.buffer); I don't think it's very nice to have the HeapAlloc() and HeapFree() on different levels of the code like that. I.e., either have both in wpp_open_mem()/wpp_close_mem() or have both in the caller. The current scheme has the allocation in the caller and the deallocation in wpp_close_mem(), except sometimes when wpp_parse() fails to call wpp_close_mem(). (Can that even happen? Looking at the source of wpp_parse() it's not clear to me how.) Also, does wpp_parse() really need the input to be zero-terminated?
Seems like I'm unable to review my code...
+ if(desc->pos + len > desc->size) len = desc->size - desc->pos; It's usually safer to write expressions like that as "if (len > desc->size - desc->pos)" because "desc->pos + len" can wrap around if len is large, while "desc->size - desc->pos" should always be safe. Perhaps the caller is expected to always pass sane values here, but it's a somewhat dangerous construction in the general case in terms of reading/writing past the end of buffers. It can also be simplified to "len = min(len, desc->size - desc->pos);" here.
Sure. Better to go with something like "len = len < (desc->size - desc->pos) ? len : desc->size - desc->pos" or creating a "min" function?
+ if(wpp_output_size + len > wpp_output_capacity) Similar issue as above.
+int wpp_close_output(void) +{ + /* trim buffer to the effective size */ + char *new_wpp_output = HeapReAlloc(GetProcessHeap(), 0, wpp_output, + wpp_output_size + 1); + if(!new_wpp_output) return 0; + wpp_output[wpp_output_size]='\0'; + return 1; +} This doesn't really make sense. The comment is misleading, because you actually grow the buffer if "wpp_output_size == wpp_output_capacity". If you didn't, you wouldn't care about HeapRealloc() failure because the worst thing that could happen was that the buffer was a bit larger than it strictly needed to be. More importantly though, you assume new_wpp_output is the same pointer as wpp_output after a successful HeapReAlloc(), which isn't necessarily true.
Yep, this is clearly broken. Will fix it.
+ current_shader.buffer = HeapAlloc(GetProcessHeap(), 0, data_len + 1); ... + ret = wpp_parse("", NULL); + if(!wpp_close_output()) + ret = 1; + if(ret) + { + TRACE("Error during shader preprocessing\n"); + HeapFree(GetProcessHeap(), 0, current_shader.buffer); I don't think it's very nice to have the HeapAlloc() and HeapFree() on different levels of the code like that. I.e., either have both in wpp_open_mem()/wpp_close_mem() or have both in the caller. The current scheme has the allocation in the caller and the deallocation in wpp_close_mem(), except sometimes when wpp_parse() fails to call wpp_close_mem(). (Can that even happen? Looking at the source of wpp_parse() it's not clear to me how.) Also, does wpp_parse() really need the input to be zero-terminated?
wpp_parse() doesn't call wpp_close_mem() if the call to pp_push_define_state() fails (can fail in out-of-memory conditions), so the extra HeapFree was there for this case. Anyway, as you noticed, there is no need to null-terminate wpp input (while there is this need for the shader parser), so the entire allocation-copy-nulltermination-deallocation is useless...
On 5 February 2010 18:19, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
Sure. Better to go with something like "len = len < (desc->size - desc->pos) ? len : desc->size - desc->pos" or creating a "min" function?
min and max are already defined in windef.h. Note that they're not safe for expressions with side-effects, but for simple expressions like the one here they're ok.
I don't think it's very nice to have the HeapAlloc() and HeapFree() on different levels of the code like that. I.e., either have both in wpp_open_mem()/wpp_close_mem() or have both in the caller. The current scheme has the allocation in the caller and the deallocation in wpp_close_mem(), except sometimes when wpp_parse() fails to call wpp_close_mem(). (Can that even happen? Looking at the source of wpp_parse() it's not clear to me how.) Also, does wpp_parse() really need the input to be zero-terminated?
wpp_parse() doesn't call wpp_close_mem() if the call to pp_push_define_state() fails (can fail in out-of-memory conditions), so the extra HeapFree was there for this case. Right, I only looked if wpp_callbacks->open()/wpp_callbacks->close() matched. If the alloc/free calls are on the same level it can't happen.
Henri Verbeet wrote:
On 5 February 2010 18:19, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
Sure. Better to go with something like "len = len < (desc->size - desc->pos) ? len : desc->size - desc->pos" or creating a "min" function?
min and max are already defined in windef.h. Note that they're not safe for expressions with side-effects, but for simple expressions like the one here they're ok. We can lift the side-effects and type safe min/max macros from the Linux Kernel. Of course Alexandre would have to accept the use of typeof(); we can do a fallback for the compilers that don't support that.
bye michael
On 5 February 2010 19:10, Michael Stefaniuc <mstefani(a)redhat.com> wrote:
Henri Verbeet wrote:
min and max are already defined in windef.h. Note that they're not safe for expressions with side-effects, but for simple expressions like the one here they're ok. We can lift the side-effects and type safe min/max macros from the Linux Kernel. Of course Alexandre would have to accept the use of typeof(); we can do a fallback for the compilers that don't support that.
I imagine it can't be *that* hard to fix the macros, but wouldn't that make them incompatible with the PSDK ones?
On 02/05/2010 07:38 PM, Henri Verbeet wrote:
On 5 February 2010 19:10, Michael Stefaniuc<mstefani(a)redhat.com> wrote:
Henri Verbeet wrote:
min and max are already defined in windef.h. Note that they're not safe for expressions with side-effects, but for simple expressions like the one here they're ok. We can lift the side-effects and type safe min/max macros from the Linux Kernel. Of course Alexandre would have to accept the use of typeof(); we can do a fallback for the compilers that don't support that.
I imagine it can't be *that* hard to fix the macros, but wouldn't that make them incompatible with the PSDK ones? Not really, you can guard them with ifdef __WINESRC__. The Wine source already has stricter requirements than what the Win32 API would allow. Of course the real issue is getting to the point where Wine will compile without warnings with the strict min/max macros. Actually that was once a janitorial task to use the strict min/max macros and fixup the problems found by that.
bye michael
participants (3)
-
Henri Verbeet -
Matteo Bruni -
Michael Stefaniuc