[PATCH vkd3d v2 06/10] vkd3d-shader/hlsl: Properly free parse_variable_def memory in declare_vars().
Zebediah Figura (she/her)
zfigura at codeweavers.com
Tue Jan 11 11:34:20 CST 2022
On 1/11/22 10:42, Giovanni Mascellani wrote:
> Hi,
>
> my understanding is the aim of this patch is to fix memory leaks that
> happen in error branches, taking care of not freeing stuff a reference
> to which was stored somewhere else. I don't like how this is achieved.
>
> First, I think it is useful to have just one clearly identified
> destructor for each type (struct parse_variable_def in this case).
> Destroying objects in many different places makes it harder to maintain
> the code when new fields are added, because it is easy to forget to
> destroy them somewhere (possibly what already happened here). So I think
> that objects of type struct parse_variable_def should always be
> destroyed in free_parse_variable_def(), as you did in your first iteration.
>
> The problem with that if some pointer was copied somewhere else, we
> mustn't free it any more. In other words, we have to make pointer
> ownership more clear. To do that, I would suggest to set pointers to
> NULL once ownership has been transferred somewhere else. So, for
> example, after a successful hlsl_new_var() you'd set v->name and
> v->semantic.name to NULL. In the destructor, vkd3d_free() is already
> able to deal with NULL pointers, ignoring them (things are a little bit
> more complicated with the "initializer" field, which is a sub-object and
> not a pointer; either we make it a pointer, or you have to set to NULL
> its fields after ownership transfer; I don't like the latter because it
> is another encapsulation violation, that is likely to eventually cause
> problems analogous to the one you're trying to solve).
>
> Be aware that there is a non-negligible probability that somebody with
> higher sign-off power than me might disagree with me.
I'm afraid I don't agree in this case. The principle is fine in general;
the problem here is that struct parse_variable_def isn't semantically an
object; it's a collection of information that's grouped into a struct
only because yacc requires it. The fact that the code ends up using it
in a relatively piecewise fashion is a good indication of this.
More information about the wine-devel
mailing list