[Mesa-dev] [PATCH] glsl: Move error message inside validation check reducing duplicate message handling

[Paul Berry]

On 17 October 2013 04:42, Timothy Arceri <t_arceri at yahoo.com.au> wrote:

> ---
>  src/glsl/ast_to_hir.cpp | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index dfa32d9..f96ed53 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -637,8 +637,8 @@ shift_result_type(const struct glsl_type *type_a,
>   */
>  ir_rvalue *
>  validate_assignment(struct _mesa_glsl_parse_state *state,
> -                   const glsl_type *lhs_type, ir_rvalue *rhs,
> -                   bool is_initializer)
> +                    YYLTYPE loc, const glsl_type *lhs_type,
> +                    ir_rvalue *rhs, bool is_initializer)
>  {
>     /* If there is already some error in the RHS, just return it.  Anything
>      * else will lead to an avalanche of error message back to the user.
> @@ -670,6 +670,12 @@ validate_assignment(struct _mesa_glsl_parse_state
> *state,
>          return rhs;
>     }
>
> +   _mesa_glsl_error(&loc, state,
> +                    is_initializer ? "initializer" : "value"
> +                    " of type %s cannot be assigned to "
> +                    "variable of type %s",
> +                    rhs->type->name, lhs_type->name);
> +
>

This doesn't produce the output you want.  String concatenation happens at
compile time and takes precedence over everything else, so this is being
interpreted as:

_mesa_glsl_error(&loc, state, is_initializer ? "initializer" : "value of
type %s cannot be assigned to variable of type %s", rhs->type->name,
lhs_type->name);

Adding parenthesis doesn't help because string concatenation only works on
string literals.  I believe what you actually want is:

   _mesa_glsl_error(&loc, state,
                    "%s of type %s cannot be assigned to "
                    "variable of type %s",
                    is_initializer ? "initializer" : "value",
                    rhs->type->name, lhs_type->name);

With that change, this patch is:

Reviewed-by: Paul Berry <stereotype441 at gmail.com>

Do you have push access?  I can push the patch for you (with this change)
if you'd like.


>     return NULL;
>  }
>
> @@ -700,10 +706,10 @@ do_assignment(exec_list *instructions, struct
> _mesa_glsl_parse_state *state,
>
>        if (unlikely(expr->operation == ir_binop_vector_extract)) {
>           ir_rvalue *new_rhs =
> -            validate_assignment(state, lhs->type, rhs, is_initializer);
> +            validate_assignment(state, lhs_loc, lhs->type,
> +                                rhs, is_initializer);
>
>           if (new_rhs == NULL) {
> -            _mesa_glsl_error(& lhs_loc, state, "type mismatch");
>              return lhs;
>           } else {
>              rhs = new(ctx) ir_expression(ir_triop_vector_insert,
> @@ -752,10 +758,8 @@ do_assignment(exec_list *instructions, struct
> _mesa_glsl_parse_state *state,
>     }
>
>     ir_rvalue *new_rhs =
> -      validate_assignment(state, lhs->type, rhs, is_initializer);
> -   if (new_rhs == NULL) {
> -      _mesa_glsl_error(& lhs_loc, state, "type mismatch");
> -   } else {
> +      validate_assignment(state, lhs_loc, lhs->type, rhs, is_initializer);
> +   if (new_rhs != NULL) {
>        rhs = new_rhs;
>
>        /* If the LHS array was not declared with a size, it takes it size
> from
> @@ -2495,7 +2499,8 @@ process_initializer(ir_variable *var,
> ast_declaration *decl,
>      */
>     if (type->qualifier.flags.q.constant
>         || type->qualifier.flags.q.uniform) {
> -      ir_rvalue *new_rhs = validate_assignment(state, var->type, rhs,
> true);
> +      ir_rvalue *new_rhs = validate_assignment(state, initializer_loc,
> +                                               var->type, rhs, true);
>        if (new_rhs != NULL) {
>          rhs = new_rhs;
>
> @@ -2524,10 +2529,6 @@ process_initializer(ir_variable *var,
> ast_declaration *decl,
>             var->constant_value = constant_value;
>          }
>        } else {
> -        _mesa_glsl_error(&initializer_loc, state,
> -                         "initializer of type %s cannot be assigned to "
> -                         "variable of type %s",
> -                         rhs->type->name, var->type->name);
>          if (var->type->is_numeric()) {
>             /* Reduce cascading errors. */
>             var->constant_value = ir_constant::zero(state, var->type);
> --
> 1.8.3.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131025/1145517f/attachment.html>