[PATCH vkd3d v2 06/10] vkd3d-shader/hlsl: Properly free parse_variable_def memory in declare_vars().

[Francisco Casas]

FWIW I agree with Giovanni in that it is good for the code maintainability that the memory 
has a clear ownership, and I think his proposal is good.

However, I accept that in this particular case it may be better to move forward and maybe
consider it as a future batch of patches if we find more uses for parse_variable_def.


January 11, 2022 2:34 PM, "Zebediah Figura (she/her)" <zfigura at codeweavers.com> wrote:

> 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.