From: "Francisco Casas" Subject: Re: [PATCH vkd3d v2 06/10] vkd3d-shader/hlsl: Properly free parse_variable_def memory in declare_vars(). Message-Id: <02d2210feb29a19395d1d3e70a457372@codeweavers.com> Date: Tue, 11 Jan 2022 18:37:35 +0000 In-Reply-To: <7075d58d-7a2f-7fd8-cc16-976dd56f0378@codeweavers.com> References: <7075d58d-7a2f-7fd8-cc16-976dd56f0378@codeweavers.com> <20220110193318.267854-6-fcasas@codeweavers.com> <9dfd4d1d-4753-4eb0-6bdd-c4318ad7933d@codeweavers.com> 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)" 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.