Re: [v9 1/6] d3dx9: Implement preshader parsing.

[Matteo Bruni]

2016-04-01 13:21 GMT+02:00 Paul Gofman :
> Signed-off-by: Paul Gofman

Looks good! Just a few tiny comments:
> +static void dump_bytecode(void *data, unsigned int size)
> +{
> - unsigned int *bytecode = (unsigned int *)data;

The cast is unnecessary.
> +static unsigned int *find_bytecode_comment(unsigned int *ptr, unsigned int count,
> -
>    unsigned int fourcc, unsigned int *size)
>
> +{
> - /* Provide at least one value in comment section on non-NULL return. */
> - while (count > 2 && (*ptr & 0xffff) == 0xfffe)
> - {
> -
>    unsigned int section_size;
> -
> -
>    section_size = (*ptr >> 16);
> -
>    if (!section_size || section_size + 1 > count)
> -
>        break;

I guess a WARN() would be nice there. Also probably an "explicit"
return NULL is a bit nicer.
> +static HRESULT parse_preshader(struct d3dx_preshader *pres, unsigned int *ptr, unsigned int count, struct d3dx9_base_effect *base)

Try to stay below 120 characters per line (targeting ~100).
> - p = find_bytecode_comment(ptr + 1, count - 1, FOURCC_CLIT, &section_size);
> - if (p)
> - {
> -
>    const_count = *p++;
> -
>    if (const_count > (section_size - 1) / (sizeof(double) / sizeof(unsigned int)))
> -
>    {
> -
>        WARN("Byte code buffer ends unexpectedly.\n");
> -
>        return D3DXERR_INVALIDDATA;
> -
>    }
> -
>    dconst = (double *)p;
> - }
> - else
> - {
> -
>    const_count = 0;
> -
>    dconst = NULL;
> - }
> - TRACE("%u double constants.\n", const_count);
> -
> - p = find_bytecode_comment(ptr + 1, count - 1, FOURCC_FXLC, &section_size);

You could decrement "count" by the amount of section_size before
looking for the next comment. It probably makes sense to set *size to
0 at the beginning of find_bytecode_comment() so that you can
unconditionally decrement here.
> - count = byte_code_size / sizeof(unsigned int);
> - if (!byte_code || !count)
> - {
> -
>    *peval_out = NULL;
> -
>    return;
> - }

It just occurred to me, you could also check for byte_code_size %
sizeof(unsigned int) while you're at it.