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.
Giovanni.
On 10/01/22 20:33, Francisco Casas wrote:
Signed-off-by: Francisco Casas fcasas@codeweavers.com
libs/vkd3d-shader/hlsl.y | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 636882c4..988e0743 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1424,6 +1424,9 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_MISSING_INITIALIZER, "Const variable "%s" is missing an initializer.", var->name); hlsl_free_var(var);
free_parse_initializer(&v->initializer);if (v->arrays.count)vkd3d_free(v->arrays.sizes); vkd3d_free(v); continue; }@@ -1436,6 +1439,9 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t "Variable "%s" was already declared in this scope.", var->name); hlsl_note(ctx, &old->loc, VKD3D_SHADER_LOG_ERROR, ""%s" was previously declared here.", old->name); hlsl_free_var(var);
free_parse_initializer(&v->initializer);if (v->arrays.count)vkd3d_free(v->arrays.sizes); vkd3d_free(v); continue; }@@ -1485,6 +1491,7 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t { hlsl_fixme(ctx, &v->loc, "Array initializer."); free_parse_initializer(&v->initializer);
vkd3d_free(v->arrays.sizes); vkd3d_free(v); continue; }@@ -1507,6 +1514,9 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t list_move_tail(statements_list, v->initializer.instrs); vkd3d_free(v->initializer.instrs); }
if (v->arrays.count)vkd3d_free(v->arrays.sizes); vkd3d_free(v); } vkd3d_free(var_list);