[Mesa-dev] [PATCH 09/16] glsl: clean-up link uniform code

[Timothy Arceri]

On Tue, 2015-07-21 at 10:03 +0200, Samuel Iglesias Gonsálvez wrote:
> On 18/07/15 03:25, Timothy Arceri wrote:
> > ---
> >  src/glsl/link_uniforms.cpp | 16 ++++++----------
> >  1 file changed, 6 insertions(+), 10 deletions(-)
> > 
> > diff --git a/src/glsl/link_uniforms.cpp b/src/glsl/link_uniforms.cpp
> > index 11ae06f..2d50b9b 100644
> > --- a/src/glsl/link_uniforms.cpp
> > +++ b/src/glsl/link_uniforms.cpp
> > @@ -71,6 +71,7 @@ void
> >  program_resource_visitor::process(ir_variable *var)
> >  {
> >     const glsl_type *t = var->type;
> > +   const glsl_type *t_without_array = var->type->without_array();
> >     const bool row_major =
> >        var->data.matrix_layout == GLSL_MATRIX_LAYOUT_ROW_MAJOR;
> >  
> > @@ -140,12 +141,8 @@ program_resource_visitor::process(ir_variable *var)
> >        char *name = ralloc_strdup(NULL, var->name);
> >        recursion(var->type, &name, strlen(name), row_major, NULL, false);
> >        ralloc_free(name);
> > -   } else if (t->is_interface()) {
> > -      char *name = ralloc_strdup(NULL, var->type->name);
> > -      recursion(var->type, &name, strlen(name), row_major, NULL, false);
> > -      ralloc_free(name);
> > -   } else if (t->is_array() && t->fields.array->is_interface()) {
> > -      char *name = ralloc_strdup(NULL, var->type->fields.array->name);
> > +   } else if (t_without_array->is_interface()) {
> > +      char *name = ralloc_strdup(NULL, t_without_array->name);
> >        recursion(var->type, &name, strlen(name), row_major, NULL, false);
> >        ralloc_free(name);
> >     } else {
> > @@ -216,8 +213,8 @@ program_resource_visitor::recursion(const glsl_type 
> > *t, char **name,
> >           (*name)[name_length] = '\0';
> >           this->leave_record(t, *name, row_major);
> >        }
> > -   } else if (t->is_array() && (t->fields.array->is_record()
> > -                                || t->fields.array->is_interface())) {
> > +   } else if (t->without_array()->is_record()
> > +              || t->without_array()->is_interface()) {
> 
> I have my doubts about this change. It is not wrong per se but I prefer
> to keep the context the original code gives to the reader.
> 
> With your change, readers might not see that we are checking an array of
> interfaces/records and they could ask themselves why this code is not
> merged to previous t->is_interface() or t->is_record() cases.
> 
> I would do only that change if we are planning to merge this code to the
> non-array case.
> 
> >        if (record_type == NULL && t->fields.array->is_record())
> >           record_type = t->fields.array;
> >  
> > @@ -778,8 +775,7 @@ link_update_uniform_buffer_variables(struct gl_shader 
> > *shader)
> >  
> >        if (var->type->is_record()) {
> >           sentinel = '.';
> > -      } else if (var->type->is_array()
> > -                 && var->type->fields.array->is_record()) {
> > +      } else if (var->type->without_array()->is_record()) {
> 
> Same doubt as before.
> 
> However, I think it is a matter of taste. What do you think?

I did have the same concern as you initially. My commit message is a little
misleading the change also adds support for AoA to structs and interfaces too.
I split this change out of a different patch and forgot to mention it (I'll
update the commit message).

I guess it's probably a good idea to add some comments to the code to make the
differences more clear, I'll send a new version with this change.

On the positive side if someone did try combine these then piglit tests would
fail pretty quickly.

Thanks for the reviews.

Tim
> 
> Sam
> 
> >           sentinel = '[';
> >        }
> >  
> >