[1/3] d3dx9: Partial implementation of D3DXAssembleShader function. [try 2]

[Henri Verbeet]

On 4 February 2010 19:26, Matteo Bruni <matteo.mystral at 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?