From: "Zebediah Figura (she/her)" <zfigura@codeweavers.com>
Subject: Re: [PATCH vkd3d v2 06/10] vkd3d-shader/hlsl: Properly free parse_variable_def memory in declare_vars().
Message-Id: <7075d58d-7a2f-7fd8-cc16-976dd56f0378@codeweavers.com>
Date: Tue, 11 Jan 2022 11:34:20 -0600
In-Reply-To: <9dfd4d1d-4753-4eb0-6bdd-c4318ad7933d@codeweavers.com>
References: <20220110193318.267854-6-fcasas@codeweavers.com> <9dfd4d1d-4753-4eb0-6bdd-c4318ad7933d@codeweavers.com>

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.