[Mesa-dev] [PATCH 2/2] glsl: struct constructors/initializers only allow implicit conversions

[Andres Gomez]

On Mon, 2016-08-01 at 12:05 +0300, Andres Gomez wrote:
> On Mon, 2016-08-01 at 10:01 +1000, Timothy Arceri wrote:
> > On Sun, 2016-07-31 at 18:43 +0300, Andres Gomez wrote:
> 
> [snip]
> 
> > diff --git a/src/compiler/glsl/ast_function.cpp
> > > b/src/compiler/glsl/ast_function.cpp
> > > index 9dcec50..9b09cb6 100644
> > > --- a/src/compiler/glsl/ast_function.cpp
> > > +++ b/src/compiler/glsl/ast_function.cpp

[snip]

> > > @@ -1719,18 +1732,35 @@ process_record_constructor(exec_list
> > > *instructions,
> > >           return ir_rvalue::error_value(ctx);
> > >        }
> > >  
> > > -      if (apply_implicit_conversion(constructor_type-
> > > > fields.structure[i].type,
> > > -                                 ir, state)) {
> > > -         node->replace_with(ir);
> > > -      } else {
> > > -         _mesa_glsl_error(loc, state,
> > > -                          "parameter type mismatch in constructor
> > > for `%s.%s' "
> > > -                          "(%s vs %s)",
> > > -                          constructor_type->name,
> > > -                          constructor_type-
> > > > fields.structure[i].name,
> > > -                          ir->type->name,
> > > -                          constructor_type-
> > > > fields.structure[i].type->name);
> > > +      const glsl_base_type element_base_type =
> > > +         constructor_type->fields.structure[i].type->base_type;
> > > +
> > > +      /* Apply implicit conversions (not the scalar constructor
> > > rules!). See
> > > +       * the spec quote above. */
> > 
> > */ should be on the next line
> 
> Right! I will update.
> 
> > > +      if (element_base_type != result->type->base_type) {
> > > +         const glsl_type *desired_type =
> > > +            glsl_type::get_instance(element_base_type,
> > > +                                    result->type->vector_elements,
> > > +                                    result->type->matrix_columns);
> > > +
> > > +	 if (result->type->can_implicitly_convert_to(desired_type,
> > > state)) {
> > > +	    /* Even though convert_component() implements the
> > > constructor
> > > +	     * conversion rules (not the implicit conversion rules),
> > > its safe
> > > +	     * to use it here because we already checked that the
> > > implicit
> > > +	     * conversion is legal.
> > > +	     */
> > > +	    result = convert_component(result, desired_type);
> > > +	 }
> > 
> > You are adding a bunch of tabs above please remove them. It would be
> > nice if you could add a forth patch to the series that removes the
> > remain tabs in this file since there are not many remaining.
> 
> Ouch! The curse of copy and paste ☹
> 
> I will update and add that 4th patch.
> 
> Now that you comment that, maybe you can give a review to?
> https://patchwork.freedesktop.org/patch/97838/
> 
> > > +      }
> > > +
> > > +      if (result->type != constructor_type-
> > > > fields.structure[i].type) {
> > > +	 _mesa_glsl_error(loc, state, "type error in array
> > > constructor: "
> > > +			  "expected: %s, found %s",
> > > +			  constructor_type-
> > > > fields.structure[i].type->name,
> > > +			  result->type->name);
> > >           return ir_rvalue::error_value(ctx);
> > > +      } else {
> > > +         node->replace_with(result);
> > >        }
> > 
> > This if-else should be inside the previous if.
> > 
> >  The node->replace_with(result); should be after
> >  result = convert_component(result, desired_type);
> > 
> > Otherwise you are replacing the node with itself.
> > 
> > The error message can be added as an else to if (result->type-
> > > can_implicitly_convert_to())
> 
> Argh! Yeah, that's a shame.
> 
> I will update.

Forgot to comment about placing the error message also nested as an
else to the conversion.

That's not OK. Since we are not attempting the convert for any type,
but only the ones with a different base_type, we still have to check
whether the type is the expected or not and, then, run the error code.

-- 

Br,

Andres