<div dir="ltr">On 21 January 2014 04:19, Timothy Arceri <span dir="ltr"><<a href="mailto:t_arceri@yahoo.com.au" target="_blank">t_arceri@yahoo.com.au</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Signed-off-by: Timothy Arceri <<a href="mailto:t_arceri@yahoo.com.au">t_arceri@yahoo.com.au</a>><br>
---<br>
src/glsl/glsl_parser.yy | 153 +++++++++++++++++++++---------------------------<br>
1 file changed, 66 insertions(+), 87 deletions(-)<br>
<br>
diff --git a/src/glsl/glsl_parser.yy b/src/glsl/glsl_parser.yy<br>
index 1c56d6f..6d63668 100644<br>
--- a/src/glsl/glsl_parser.yy<br>
+++ b/src/glsl/glsl_parser.yy<br>
@@ -1611,19 +1565,55 @@ storage_qualifier:<br>
}<br>
;<br>
<br>
-type_specifier:<br>
- type_specifier_nonarray<br>
- | type_specifier_nonarray '[' ']'<br>
+array_specifier:<br>
+ '[' ']'<br>
{<br>
- $$ = $1;<br>
- $$->is_array = true;<br>
- $$->array_size = NULL;<br>
+ void *ctx = state;<br>
+ $$ = new(ctx) ast_array_specifier();<br>
+ $$->is_unsized_array = true;<br>
+ $$->set_location(yylloc);<br>
+ }<br>
+ | '[' constant_expression ']'<br>
+ {<br>
+ void *ctx = state;<br>
+ $$ = new(ctx) ast_array_specifier();<br>
+ $$->array_dimensions.push_tail(& $2->link);<br>
+ $$->is_unsized_array = false;<br>
+ $$->set_location(yylloc);<br>
+ }<br>
+ | array_specifier '[' ']'<br>
+ {<br>
+ if (!state->ARB_arrays_of_arrays_enable) {<br>
+ _mesa_glsl_error(& @1, state,<br>
+ "#version 120 / GL_ARB_arrays_of_arrays "<br>
+ "required for defining arrays of arrays");<br></blockquote><div><br><div>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.<br></div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ } else {<br>
+ _mesa_glsl_error(& @1, state,<br>
+ "only the outermost array dimension can "<br>
+ "be unsized");<br>
+ }<br></blockquote><div><br></div><div>To prevent uninitialized data in the error condition, I'd recommend doing<br><br>$$ = $1;<br><br></div><div>here. Otherwse we might crash before the user can see the error message.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
}<br>
- | type_specifier_nonarray '[' constant_expression ']'<br>
+ | array_specifier '[' constant_expression ']'<br>
+ {<br>
+ if (!state->ARB_arrays_of_arrays_enable) {<br>
+ _mesa_glsl_error(& @1, state,<br>
+ "#version 120 / GL_ARB_arrays_of_arrays "<br>
+ "required for defining arrays of arrays");<br></blockquote><div><br></div><div>Similar comment about dropping "#version 120" from the error message here.<br></div><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ }<br>
+<br></blockquote><div><br></div><div>I think we need to add:<br><br>$$ = $1;<br><br></div><div>here.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ $$->array_dimensions.push_tail(& $3->link);<br>
+ $$->dimension_count++;<br>
+ $$->set_location(yylloc);<br></blockquote><div><br></div><div>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.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ }<br>
+ ;<br>
+<br>
+type_specifier:<br>
+ type_specifier_nonarray<br>
+ | type_specifier_nonarray array_specifier<br>
{<br>
$$ = $1;<br>
$$->is_array = true;<br>
- $$->array_size = $3;<br>
+ $$->array_specifier = $2;<br>
}<br>
;<br></blockquote><div><br></div><div>With those changes, this patch is:<br><br></div><div>Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>><br><br>Note, however, that some of my review comments on patch 3/8 will probably affect the content of this patch.<br>
</div></div></div></div>