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.