Re: [PATCH v2 4/4] d3dx9: Implement ID3DXEffect::CloneEffect().

[Zebediah Figura]

On 2/12/22 13:54, Matteo Bruni wrote:
> > @@ -199,6 +199,10 @@ struct d3dx_effect
> >   struct list parameter_block_list;
> >   struct d3dx_parameter_block *current_parameter_block;
> > -
> > - char *source;
> > - SIZE_T source_size;
> > - char *skip_constants_string;
>
> Cloning the "skip constants" setting seems reasonable but I'd prefer
> to have a test confirming that.

Done in v3.
> > -
> >    case D3DXPT_SAMPLER:
> > -
> >    case D3DXPT_SAMPLER1D:
> > -
> >    case D3DXPT_SAMPLER2D:
> > -
> >    case D3DXPT_SAMPLER3D:
> > -
> >    case D3DXPT_SAMPLERCUBE:
> > -
> >    case D3DXPT_PIXELSHADER:
> > -
> >    case D3DXPT_VERTEXSHADER:
> > -
> >        /* Nothing to do; these parameters are not mutable. */
> > -
> >        break;
>
> The test doesn't confirm that those are just recreated from scratch
> (rather than e.g. shared with the original effect). Can we add some
> tests for this?

Shaders are in fact shared if possible. Unless I'm misreading though, 
there's no way for samplers to be shared.
> > - dst->manager = src->manager;
> > - if (dst->manager)
> > -
> >    dst->manager->lpVtbl->AddRef(dst->manager);
>
> Again, this makes perfect sense but a test would make it foolproof.

This one, surprisingly enough, is wrong.
> > - if (!(effect->source = malloc(data_size)))
> > -
> >    return E_OUTOFMEMORY;
> > - memcpy(effect->source, data, data_size);
> > - effect->source_size = data_size;
> > -
>
> We should probably do this only if D3DXFX_NOT_CLONEABLE is not set.
> >   if (pool)
> >   {
> >       effect->pool = unsafe_impl_from_ID3DXEffectPool(pool);
> >
> > @@ -6429,6 +6546,12 @@ static HRESULT d3dx9_effect_init_from_dxbc(struct d3dx_effect *effect,
> >   if (skip_constants_string)
> >   {
> > -
> >    if (!(effect->skip_constants_string = strdup(skip_constants_string)))
> > -
> >    {
> > -
> >        hr = E_OUTOFMEMORY;
> > -
> >        goto fail;
> > -
> >    }
> > -
>
> Same here.

Both done in v3.