[Mesa-dev] [PATCH V2] glsl: remove unnecessary check

[Timothy Arceri]

On Sat, 2014-07-19 at 12:25 +1000, Timothy Arceri wrote:
> This code does nothing useful as the next recursive call on the array element
> will override any null values if the element is a record anyway. The code is
> also not doing what the comment says as its trying to set the record type
> pointer for only the first element of the array not the first leaf field 
> of the record.

It's been a while since I wrote this patch so I decided to double check
it. For a second I thought I should have maybe left the null assignment
in the array loop, so that if record_type was already passed it wouldn't
be passed to each array element but that's what happens to each element
anyway if record_type is null.

For example:

struct S1 { 
   vec4 v;
   float f;
};

struct S { 
   S1 s1[3];
   S1 s2;
};

uniform Ubo { 
   S s;
};


Without my patch:

s.s1[0].v == record_type: S
s.s1[1].v == record_type: S1
s.s1[2].v == record_type: S1
s.s1[0].f == record_type: null
s.s1[1].f == record_type: null
s.s1[2].f == record_type: null
s.s2.v == record_type: S1
s.s2.f == record_type: null

With my patch:

s.s1[0].v == record_type: S
s.s1[1].v == record_type: S
s.s1[2].v == record_type: S
s.s1[0].f == record_type: null
s.s1[1].f == record_type: null
s.s1[2].f == record_type: null
s.s2.v == record_type: S1
s.s2.v == record_type: null

If this change is not desirable then leaving record_type = NULL; in the
array loop will restore the previous behaviour. However the removal of
of setting the record_type seems valid either way. 


> 
> Signed-off-by: Timothy Arceri <t_arceri at yahoo.com.au>
> ---
> 
> V2: Make commit message lines shorter than  80 characters
> 
> Resending this patch. Its a required cleanup for arrays of arrays.
> 
> The code was added as part of commit 684316512c5570483365d36849250a008b6fcd84
> Ian maybe you can comment on whether I'm following correctly?
> 
>  src/glsl/link_uniforms.cpp | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/src/glsl/link_uniforms.cpp b/src/glsl/link_uniforms.cpp
> index ba66053..9566124 100644
> --- a/src/glsl/link_uniforms.cpp
> +++ b/src/glsl/link_uniforms.cpp
> @@ -187,9 +187,6 @@ program_resource_visitor::recursion(const glsl_type *t, char **name,
>        }
>     } else if (t->is_array() && (t->fields.array->is_record()
>                                  || t->fields.array->is_interface())) {
> -      if (record_type == NULL && t->fields.array->is_record())
> -         record_type = t->fields.array;
> -
>        for (unsigned i = 0; i < t->length; i++) {
>  	 size_t new_length = name_length;
>  
> @@ -198,11 +195,6 @@ program_resource_visitor::recursion(const glsl_type *t, char **name,
>  
>           recursion(t->fields.array, name, new_length, row_major,
>                     record_type);
> -
> -         /* Only the first leaf-field of the record gets called with the
> -          * record type pointer.
> -          */
> -         record_type = NULL;
>        }
>     } else {
>        this->visit_field(t, *name, row_major, record_type);