Re: [v2 2/3] d3dx9: Implement IsParameterUsed function in effect.

[Matteo Bruni]

2016-05-23 12:40 GMT+02:00 Paul Gofman :
> Signed-off-by: Paul Gofman

It looks generally good, I only have minor "complaints" (and sorry for
the delay in reviewing...).
> dlls/d3dx9_36/effect.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 110 insertions(+), 3 deletions(-)
>
> diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c
> index 31aa2d6..cdfc494 100644
> --- a/dlls/d3dx9_36/effect.c
> +++ b/dlls/d3dx9_36/effect.c
> @@ -3545,15 +3545,122 @@ static HRESULT WINAPI ID3DXEffectImpl_FindNextValidTechnique(ID3DXEffect* iface,
>      return E_NOTIMPL;
>  }
>
> +typedef BOOL (*walk_parameter_dep_func)(void *data, struct d3dx_parameter *param);
> +static BOOL walk_state_dep(struct d3dx_state *state, walk_parameter_dep_func param_func,
> -
>    void *data);
>
> +static BOOL walk_param_eval_dep(struct d3dx_param_eval *param_eval, walk_parameter_dep_func param_func,
> -
>    void *data);

I guess you intend to reuse all these functions down the line for some
other API, what are your plans?
> +static BOOL compare_param_ptr(void *param_comp, struct d3dx_parameter *param)
> +{
> - return param_comp == (void *)param;

I think the cast is unnecessary here.
> +}
> -
>
> static BOOL WINAPI ID3DXEffectImpl_IsParameterUsed(ID3DXEffect* iface, D3DXHANDLE parameter, D3DXHANDLE technique)
>  {
>      struct ID3DXEffectImpl *effect = impl_from_ID3DXEffect(iface);
> - unsigned int i, j;
> struct d3dx_parameter *param = get_valid_parameter(&effect->base_effect, parameter);
> - struct d3dx_technique *tech = get_valid_technique(&effect->base_effect, technique);
> - struct d3dx_pass *pass;
> - FIXME("iface %p, parameter %p, technique %p stub.\n", iface, parameter, technique);
> - TRACE("param %p (%s).\n", param, param ? debugstr_a(param->name) : "");
> - TRACE("iface %p, parameter %p, technique %p.\n", iface, parameter, technique);
> - TRACE("param %p, name %s, tech %p.\n", param, param ? debugstr_a(param->name) : "", tech);
> - if (!tech || !param)
> -
>    return FALSE;
>
> - return TRUE;
> - for (i = 0; i < tech->pass_count; ++i)
> - {
> -
>    pass = &tech->passes[i];
> -
>    for (j = 0; j < pass->state_count; ++j)
> -
>    {
> -
>        if (walk_state_dep(&pass->states[j], compare_param_ptr, param))
> -
>        {
> -
>            TRACE("technique %p, parameter %s, TRUE.\n", tech, param ? debugstr_a(param->name) : "");

I don't think you need to trace technique and parameter again, they
are in the immediately preceding ID3DXEffectImpl_IsParameterUsed()
TRACE anyway. Maybe the message could simply be "Returning TRUE.\n" or
similar.
> -
>            return TRUE;
> -
>        }
> -
>    }
> - }
> - TRACE("technique %p, parameter %s, FALSE.\n", tech, param ? debugstr_a(param->name) : "");

Same here.
> - return FALSE;
>
> }
> static HRESULT WINAPI ID3DXEffectImpl_Begin(ID3DXEffect *iface, UINT *passes, DWORD flags)
>
> 2.5.5