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

[Matteo Bruni]

2016-04-01 13:21 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
> Signed-off-by: Paul Gofman <gofmanp(a)gmail.com>

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, §ion_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, §ion_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.