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

[Matteo Bruni]

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...