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

> From: Ian Romanick <ian.d.roman...@intel.com>
>
> Signed-off-by: Ian Romanick <ian.d.roman...@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@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 <stereotype...@gmail.com>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to