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

[Paul Berry]

On 22 January 2013 00:52, Ian Romanick <idr at freedesktop.org> wrote:

> From: Ian Romanick <ian.d.romanick at intel.com>
>
> Signed-off-by: Ian Romanick <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
> 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.

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.

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>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130123/d70ae551/attachment.html>