[Mesa-dev] [PATCH 19/32] glsl: Handle instance array declarations

[Kenneth Graunke]

On 01/23/2013 07:53 PM, Paul Berry wrote:
> On 22 January 2013 00:52, Ian Romanick <idr at freedesktop.org
> <mailto:idr at freedesktop.org>> wrote:
>
>     From: Ian Romanick <ian.d.romanick at intel.com
>     <mailto:ian.d.romanick at intel.com>>
>
>     Signed-off-by: Ian Romanick <ian.d.romanick at intel.com
>     <mailto:ian.d.romanick at intel.com>>
>     ---
>       src/glsl/ast_to_hir.cpp | 17 ++++++++++++++---
>       1 file changed, 14 insertions(+), 3 deletions(-)
>
>     diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>     index d485bc8..c922a84 100644
>     --- a/src/glsl/ast_to_hir.cpp
>     +++ b/src/glsl/ast_to_hir.cpp
>     @@ -4259,9 +4259,20 @@ ast_uniform_block::hir(exec_list *instructions,
>           *     field selector ( . ) operator (analogously to structures)."
>           */
>          if (this->instance_name) {
>     -      ir_variable *var = new(state) ir_variable(block_type,
>     -                                                this->instance_name,
>     -                                                ir_var_uniform);
>     +      ir_variable *var;
>     +
>     +      if (this->array_size != NULL) {
>     +         const glsl_type *block_array_type =
>     +            process_array_type(&loc, block_type, this->array_size,
>     state);
>     +
>     +         var = new(state) ir_variable(block_array_type,
>     +                                      this->instance_name,
>     +                                      ir_var_uniform);
>     +      } else {
>     +         var = new(state) ir_variable(block_type,
>     +                                      this->instance_name,
>     +                                      ir_var_uniform);
>     +      }
>
>             var->interface_type = block_type;
>             state->symbols->add_variable(var);
>     --
>     1.7.11.7
>
>     _______________________________________________
>     mesa-dev mailing list
>     mesa-dev at lists.freedesktop.org <mailto:mesa-dev at lists.freedesktop.org>
>     http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
> Immediately below this hunk there's an else branch, to deal with the
> case where the uniform block doesn't have an instance name.  Why don't
> we need to add similar array-handling logic to the else branch?  I'm
> guessing that the grammar prevents instance blocks without names from
> being arrays, but since I'm not too familiar with UBO's I'm not very
> certain about it.

Yes, that's the case.  It makes sense, too, because blocks without 
instance names just have variables at global scope...so there's nothing 
sensible to put an array subscript on.

> If my guess is right, it would be nice to put an explanatory comment in
> the else branch, and maybe an "assert(this->array_size == NULL);" just
> to drive the point home.

Certainly never hurts.

> But I won't be a stickler about it.  With or without my suggested
> change, this patch is:
>
> Reviewed-by: Paul Berry <stereotype441 at gmail.com
> <mailto:stereotype441 at gmail.com>>
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>