[Mesa-dev] [PATCH V2 2/8] glsl: Add arrays_of_arrays to yacc definition

[Paul Berry]

On 21 January 2014 04:19, Timothy Arceri <t_arceri at yahoo.com.au> wrote:

> Signed-off-by: Timothy Arceri <t_arceri at yahoo.com.au>
> ---
>  src/glsl/glsl_parser.yy | 153
> +++++++++++++++++++++---------------------------
>  1 file changed, 66 insertions(+), 87 deletions(-)
>
> diff --git a/src/glsl/glsl_parser.yy b/src/glsl/glsl_parser.yy
> index 1c56d6f..6d63668 100644
> --- a/src/glsl/glsl_parser.yy
> +++ b/src/glsl/glsl_parser.yy
> @@ -1611,19 +1565,55 @@ storage_qualifier:
>     }
>     ;
>
> -type_specifier:
> -   type_specifier_nonarray
> -   | type_specifier_nonarray '[' ']'
> +array_specifier:
> +   '[' ']'
>     {
> -      $$ = $1;
> -      $$->is_array = true;
> -      $$->array_size = NULL;
> +      void *ctx = state;
> +      $$ = new(ctx) ast_array_specifier();
> +      $$->is_unsized_array = true;
> +      $$->set_location(yylloc);
> +   }
> +   | '[' constant_expression ']'
> +   {
> +      void *ctx = state;
> +      $$ = new(ctx) ast_array_specifier();
> +      $$->array_dimensions.push_tail(& $2->link);
> +      $$->is_unsized_array = false;
> +      $$->set_location(yylloc);
> +   }
> +   | array_specifier '[' ']'
> +   {
> +      if (!state->ARB_arrays_of_arrays_enable) {
> +         _mesa_glsl_error(& @1, state,
> +                          "#version 120 / GL_ARB_arrays_of_arrays "
> +                          "required for defining arrays of arrays");
>

This error message is confusing.  It could be reasonably interpreted as
saying that either version 120 or GL_ARB_arrays_of_arrays is sufficient to
allow arrays of arrays, which isn't true.  I would just drop the mention of
version 120 entirely--the only reason it's significant at all is that
version 120 is the first version of GLSL to mention that arrays of arrays
aren't allowed, and that was just because it was left out of version 110 as
an oversight.



> +      } else {
> +         _mesa_glsl_error(& @1, state,
> +                          "only the outermost array dimension can "
> +                          "be unsized");
> +      }
>

To prevent uninitialized data in the error condition, I'd recommend doing

$$ = $1;

here.  Otherwse we might crash before the user can see the error message.


>     }
> -   | type_specifier_nonarray '[' constant_expression ']'
> +   | array_specifier '[' constant_expression ']'
> +   {
> +      if (!state->ARB_arrays_of_arrays_enable) {
> +         _mesa_glsl_error(& @1, state,
> +                          "#version 120 / GL_ARB_arrays_of_arrays "
> +                          "required for defining arrays of arrays");
>

Similar comment about dropping "#version 120" from the error message here.


> +      }
> +
>

I think we need to add:

$$ = $1;

here.


> +      $$->array_dimensions.push_tail(& $3->link);
> +      $$->dimension_count++;
> +      $$->set_location(yylloc);
>

Assuming I'm right about adding "$$ = $1;" above, there's no need to call
set_location() here.  The location was already set when the
ast_array_specifier was created.


> +   }
> +   ;
> +
> +type_specifier:
> +   type_specifier_nonarray
> +   | type_specifier_nonarray array_specifier
>     {
>        $$ = $1;
>        $$->is_array = true;
> -      $$->array_size = $3;
> +      $$->array_specifier = $2;
>     }
>     ;
>

With those changes, this patch is:

Reviewed-by: Paul Berry <stereotype441 at gmail.com>

Note, however, that some of my review comments on patch 3/8 will probably
affect the content of this patch.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140121/68c4645a/attachment.html>