[Mesa-dev] [PATCH] glsl: emit a specific error when ast_*_assign changes type

[Timothy Arceri]

On Fri, 2016-07-08 at 23:30 -0400, Ilia Mirkin wrote:
> For regular ast_add, we can implicitly change either a or b's type.
> However in an assignment situation, the type of the lvalue is fixed.
> So
> if the implicit conversion logic decides to change it, it means that
> the
> rhs's type could not be converted to the lhs type.
> 
> Emit a specific error for this rather than the rather mysterious "is
> not
> an lvalue" error that results from having a i2f or other operation as
> the lvalue.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96729
> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> ---
>  src/compiler/glsl/ast_to_hir.cpp | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/src/compiler/glsl/ast_to_hir.cpp
> b/src/compiler/glsl/ast_to_hir.cpp
> index 8ddc084..55ee840 100644
> --- a/src/compiler/glsl/ast_to_hir.cpp
> +++ b/src/compiler/glsl/ast_to_hir.cpp
> @@ -1353,7 +1353,7 @@ ast_expression::do_hir(exec_list *instructions,
>     };
>     ir_rvalue *result = NULL;
>     ir_rvalue *op[3];
> -   const struct glsl_type *type; /* a temporary variable for switch
> cases */
> +   const struct glsl_type *type, *orig_type;
>     bool error_emitted = false;
>     YYLTYPE loc;
>  
> @@ -1639,10 +1639,18 @@ ast_expression::do_hir(exec_list
> *instructions,
>        op[0] = this->subexpressions[0]->hir(instructions, state);
>        op[1] = this->subexpressions[1]->hir(instructions, state);
>  
> +      orig_type = op[0]->type;
>        type = arithmetic_result_type(op[0], op[1],
>                                      (this->oper == ast_mul_assign),
>                                      state, & loc);
>  
> +      if (type != orig_type) {
> +         _mesa_glsl_error(& loc, state,
> +                          "could not implicitly convert "
> +                          "rvalue type to lvalue type");

Why not do:

"%s to %s", type->name, orig_type->name);

Otherwise this looks good to me.

Reviewed-by: Timothy Arceri <timothy.arceri at collabora.com>

> +         type = glsl_type::error_type;
> +      }
> +
>        ir_rvalue *temp_rhs = new(ctx) ir_expression(operations[this-
> >oper], type,
>                                                     op[0], op[1]);
>  
> @@ -1666,8 +1674,16 @@ ast_expression::do_hir(exec_list
> *instructions,
>        op[0] = this->subexpressions[0]->hir(instructions, state);
>        op[1] = this->subexpressions[1]->hir(instructions, state);
>  
> +      orig_type = op[0]->type;
>        type = modulus_result_type(op[0], op[1], state, &loc);
>  
> +      if (type != orig_type) {
> +         _mesa_glsl_error(& loc, state,
> +                          "could not implicitly convert "
> +                          "rvalue type to lvalue type");
> +         type = glsl_type::error_type;
> +      }
> +
>        assert(operations[this->oper] == ir_binop_mod);
>  
>        ir_rvalue *temp_rhs;
> @@ -1707,7 +1723,17 @@ ast_expression::do_hir(exec_list
> *instructions,
>        this->subexpressions[0]->set_is_lhs(true);
>        op[0] = this->subexpressions[0]->hir(instructions, state);
>        op[1] = this->subexpressions[1]->hir(instructions, state);
> +
> +      orig_type = op[0]->type;
>        type = bit_logic_result_type(op[0], op[1], this->oper, state,
> &loc);
> +
> +      if (type != orig_type) {
> +         _mesa_glsl_error(& loc, state,
> +                          "could not implicitly convert "
> +                          "rvalue type to lvalue type");
> +         type = glsl_type::error_type;
> +      }
> +
>        ir_rvalue *temp_rhs = new(ctx) ir_expression(operations[this-
> >oper],
>                                                     type, op[0],
> op[1]);
>        error_emitted =