From: Giovanni Mascellani <gmascellani@codeweavers.com>
Subject: Re: [PATCH vkd3d 4/8] vkd3d-shader/hlsl: Prepare to swap switch order on fold_constants().
Message-Id: <1e454e8f-662f-9c16-24df-d7d4cd8acb4f@codeweavers.com>
Date: Fri, 7 Jan 2022 10:31:54 +0100
In-Reply-To: <20220106173949.82958-4-fcasas@codeweavers.com>
References: <20220106173949.82958-4-fcasas@codeweavers.com>

Hi,

under the same "code clean at all times" rule of my last email, I don't 
think we want to have this intermediate state. From what I've seen, 
usually boilerplate code is to be added with the first "useful" code 
that uses it (and removed with the last "useful" code that uses it).

Personally I'd also say that you can just convert the code in just one 
commit, without creating a commit for each single operation, which would 
let you drop the "goto fallback" trick. After all it's just four 
operations, not that hard to check. But I'm not sure others will agree 
with this.

Giovanni.


On 06/01/22 18:39, Francisco Casas wrote:
> Signed-off-by: Francisco Casas <fcasas@codeweavers.com>
> 
> ---
> 
> Don't mind about the 'goto fallback' too much, it is used transitionally
> so that the tests still pass. It is removed in later patches.
> 
> Signed-off-by: Francisco Casas <fcasas@codeweavers.com>
> ---
>   libs/vkd3d-shader/hlsl_constant_ops.c | 23 +++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
> 
> diff --git a/libs/vkd3d-shader/hlsl_constant_ops.c b/libs/vkd3d-shader/hlsl_constant_ops.c
> index 924e0380..3a778837 100644
> --- a/libs/vkd3d-shader/hlsl_constant_ops.c
> +++ b/libs/vkd3d-shader/hlsl_constant_ops.c
> @@ -25,6 +25,7 @@ bool fold_constants(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void *cont
>       struct hlsl_ir_constant *arg1, *arg2 = NULL, *res;
>       struct hlsl_ir_expr *expr;
>       unsigned int i, dimx;
> +    bool success;
>   
>       if (instr->type != HLSL_IR_EXPR)
>           return false;
> @@ -44,6 +45,28 @@ bool fold_constants(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void *cont
>           return false;
>       init_node(&res->node, HLSL_IR_CONSTANT, instr->data_type, instr->loc);
>   
> +    success = false;
> +
> +    switch (expr->op)
> +    {
> +        default:
> +            goto fallback;
> +    }
> +
> +    if (success)
> +    {
> +        list_add_before(&expr->node.entry, &res->node.entry);
> +        hlsl_replace_node(&expr->node, &res->node);
> +        return true;
> +    }
> +    else
> +    {
> +        vkd3d_free(res);
> +        return false;
> +    }
> +
> +fallback:
> +
>       switch (instr->data_type->base_type)
>       {
>           case HLSL_TYPE_FLOAT: