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: <76adbcfd-af66-3719-9f3f-ae6fae578eb8@codeweavers.com>
Date: Tue, 11 Jan 2022 12:44:24 -0600
In-Reply-To: <81ec4129-7b82-09fd-65fb-c844f579abd3@codeweavers.com>
References: <20220110193318.267854-6-fcasas@codeweavers.com> <9dfd4d1d-4753-4eb0-6bdd-c4318ad7933d@codeweavers.com> <7075d58d-7a2f-7fd8-cc16-976dd56f0378@codeweavers.com> <81ec4129-7b82-09fd-65fb-c844f579abd3@codeweavers.com>

On 1/11/22 12:23, Giovanni Mascellani wrote:
> Hi,
> 
> On 11/01/22 18:34, Zebediah Figura (she/her) wrote:
>> 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.
> 
> It is of course a completely legitimate point of view, but I think it's 
> more prone to mistakes: for instance, the many missing calls to free() 
> that Francisco is fixing.

I'm not convinced that treating it as an object would really help here. 
Leaks are just hard.

> Also, if we insist in keeping this point of view, then we should remove 
> free_parse_variable_def(), because that's precisely one of the gadgets 
> that indicates that the structure is meant to be an encapsulated object.

I don't see anything wrong with having helpers like that.