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