From: "Zebediah Figura (she/her)" Subject: Re: [PATCH vkd3d] vkd3d-shader/hlsl: Support array initializers. Message-Id: <4b188ac9-69d2-eaac-7984-881aec4ac55f@codeweavers.com> Date: Thu, 30 Dec 2021 11:54:54 -0600 In-Reply-To: <20211229145135.123336-1-fcasas@codeweavers.com> References: <20211229145135.123336-1-fcasas@codeweavers.com> On 12/29/21 08:51, Francisco Casas wrote: > The new function initialize_var_recursive() "pops" arguments from the parse_variable_def > initializer to initialize the members of var for both struct and array initialization recursively. > initializer_i is used to keep track of the index of the next input parameter to read. > > This approach should scale well for nested arrays and structs. I don't think recursion should be necessary at all. Rather, since the "offset" in hlsl_deref is designed not to care about the underlying division of the HLSL struct or array type, we should be able to do something as simple as iterating over initializer elements and sequentially storing them to the variable. In that case we don't even need to care about the type class, and can even use this code for numeric types. Some more things I'd like to see: * Tests for initializers that are too large or too small. * Tests for struct initializers, to make sure that they aren't being broken by this patch. I think we have numeric type initializer tests already but if not we should add some of those too. Splitting out the tests into separate patches (and probably ordering them before the fix) is generally appreciated. > > Signed-off-by: Francisco Casas > > --- > > The new definition of free_parse_variable_def() is unorthodox, but I > think it helps illustrate how the parse_variable_def memory should be > free on the different scenarios. > The exercise of writting it this way allowed to find a memory leak. Yes, I'm sorry, but it's very ugly. It also begs the question of why you need to change this call at all; implementing initializers should only be a matter of adding new instructions, not changing the way variables are defined.