December 30, 2021 2:55 PM, "Zebediah Figura (she/her)" zfigura@codeweavers.com wrote:
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.
I understand that hlsl_deref doesn't care about the division of the struct, however, I still think recursion is necessary.
So far, an hlsl_type is a recursive data type: It could be an HLSL_CLASS_STRUCT that contains a HLSL_CLASS_ARRAY field, that contains HLSL_CLASS_STRUCT elements, and so on (not necessarily switching between arrays and structs). As in:
struct aaa { struct { int2 ccc; float4 ddd; } bbbs[3]; };
float4 PSMain() : SV_TARGET { struct aaa foo = {11,12,13,14,15,16,21,22,23,24,25,26,31,32,33,34,35,36}; return foo.bbbs[1].ddd; // 23.0, 24.0, 25.0, 26.0 }
So, to iterate all its leaves I would eventually need to recurse (or alternatively, to work with a "stack" of type pointers which is similar). And it is necessary to do that to know the base_type of each field/element to add the right implicit cast.
I think I can subjectively improve the quality of the code, e.g. eliminating the need for initializer_i, but not abandoning recursion unless we only want to implement arrays of basic types or of completely "flat" structs.
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.
Will do, thanks for the tip.
Signed-off-by: Francisco Casas fcasas@codeweavers.com
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.
Okay, I will keep the original function and just put the right calls to release memory on each place.