Re: [PATCH 7/7] d3dx9: Support skip_constants parameter for effect.

[Matteo Bruni]

2017-07-06 13:15 GMT+02:00 Paul Gofman :
> HRESULT d3dx_create_param_eval(struct d3dx9_base_effect *base_effect, void *byte_code,
>          unsigned int byte_code_size, D3DXPARAMETER_TYPE type,
> -
>    struct d3dx_param_eval **peval, ULONG64 *version_counter) DECLSPEC_HIDDEN;
> -
>    struct d3dx_param_eval **peval, ULONG64 *version_counter,
> -
>    const char **skip_constants_names, unsigned int skip_constant_name_count) DECLSPEC_HIDDEN;

I think you can get rid of the "_name" part in those parameters, for brevity.
> +static HRESULT parse_skip_constants_string(char *skip_constants, const char ***names,
> -
>    unsigned int *name_count)

It's a bit a matter of personal taste but triple pointers look quite
ugly to me. In this case you could e.g. return "names" in place of
HRESULT.
> +{
> - const char *name;
> - char *s;
> - unsigned int size = INITIAL_CONST_NAMES_SIZE;
> -
> - *names = HeapAlloc(GetProcessHeap(), 0, sizeof(*names) * size);
> - if (!*names)
> -
>    return E_OUTOFMEMORY;
> -
> - *name_count = 0;
> - s = skip_constants;
> - while ((name = next_valid_constant_name(&s)))
> - {
> -
>    if (*name_count == size)
> -
>    {
> -
>        size *= 2;
> -
>        *names = HeapReAlloc(GetProcessHeap(), 0, *names, sizeof(*names) * size);
> -
>        if (!*names)
> -
>            return E_OUTOFMEMORY;

I think you're leaking memory on failure here.
> -
>    }
> -
>    (*names)[(*name_count)++] = name;
> - }
> - *names = HeapReAlloc(GetProcessHeap(), 0, *names, *name_count * sizeof(*names));
> - if (!*names)
> -
>    return E_OUTOFMEMORY;

And here.
> - for (i = 0; i < skip_constants_name_count; ++i)
> - {
> -
>    struct d3dx_parameter *param;
> -
> -
>    param = get_parameter_by_name(base, NULL, skip_constants_names[i]);
> -
>    if (param)
> -
>    {
> -
>        for (j = 0; j < base->technique_count; ++j)
> -
>        {
> -
>            if (is_parameter_used(param, &base->techniques[j]))
> -
>            {
> -
>                WARN("Parameter %s found in skip_constants is used in technique %u.\n",
> -
>                        debugstr_a(skip_constants_names[i]), j);
> -
>                HeapFree(GetProcessHeap(), 0, skip_constants_buffer);
> -
>                HeapFree(GetProcessHeap(), 0, skip_constants_names);
> -
>                d3dx9_base_effect_cleanup(base);
> -
>                return D3DERR_INVALIDCALL;
> -
>            }
> -
>        }
> -
>    }
> -
>    else
> -
>    {
> -
>        TRACE("Parameter %s found in skip_constants not found.\n",
> -
>                debugstr_a(skip_constants_names[i]));

This one sounds a bit awkward ("found... not found"). Maybe just
rewrite that as "skip_constants parameter %s not found". You could
apply the same trick to the previous message too.
> -
>    for (j = 0; j < skip_constant_name_count; ++j)
> -
>    {
> -
>        if (!strcmp(cdesc[index].Name, skip_constants_names[j]))

I assume the skip_constants thing isn't used often and, when it is,
very few constants (1-3) are in the list. If that's not the case, we
might want to use an rbtree for the constant names.