[Piglit] [PATCH] tests/spec: ARB_arrays_of_arrays compiler tests

[Timothy Arceri]

On Fri, 2013-10-18 at 10:26 -0700, Paul Berry wrote:
> On 5 October 2013 04:21, Timothy Arceri <t_arceri at yahoo.com.au> wrote:


> General comment: it seems redundant to have "arb_arrays_of_arrays" in
> the folder name and "array-of-array-" as a prefix to each parser test.
> Can we remove "array-of-array-" from the parser test file names (so
> for example
> tests/spec/arb_arrays_of_arrays/compiler/array-of-array-uniform.vert
> becomes tests/spec/arb_arrays_of_arrays/compiler/uniform.vert).

Sure.

> 
> Calling this test "new syntax" is confusing.  All three of these
> syntaxes are new:
> 
> 
> vec4[3][2] a;
> vec4[2] a[3];
> 
> vec4 a[3][2];
> 
> 
> I'd suggest a naming convention that just names the syntax elements in
> the order they appear.  So for instance:
> 
> 
> vec4[3][2] a; // tested by *-array-array-var
> 
> vec4[2] a[3]; // tested by *-array-var-array
> 
> vec4 a[3][2]; // tested by *-var-array-array

Ok, yes I like this much better. I didn't really like the other naming
much either I just copied it from some of the existing glsl tests that
test for a fail against this format.
> 
>  
>         @@ -0,0 +1,17 @@
>         +/* [config]
>         + * expect_result: pass
>         + * glsl_version: 1.20
>         + * require_extensions: GL_ARB_arrays_of_arrays
>         + * [end config]
>         + */
>         +#version 120
>         +#extension GL_ARB_arrays_of_arrays: enable
>         +
>         +attribute vec4 vert;
>         +
>         +void foo(vec4 [2] x[2]);
>         +
>         +void main()
>         +{
> 
> 
> It would be nice to include a call to the function here in main(), so
> that we can verify that the compiler's type checking logic correctly
> deduces that the function can be called.

Sure.
> 
>  
>         +  gl_Position = vert;
>         +}
>         diff --git
>         a/tests/spec/arb_arrays_of_arrays/compiler/array-of-array-function-parameter-declaration.vert b/tests/spec/arb_arrays_of_arrays/compiler/array-of-array-function-parameter-declaration.vert
>         new file mode 100644
>         index 0000000..60fec71
>         --- /dev/null
>         +++
>         b/tests/spec/arb_arrays_of_arrays/compiler/array-of-array-function-parameter-declaration.vert
>         @@ -0,0 +1,17 @@
>         +/* [config]
>         + * expect_result: pass
>         + * glsl_version: 1.20
>         + * require_extensions: GL_ARB_arrays_of_arrays
>         + * [end config]
>         + */
>         +#version 120
>         +#extension GL_ARB_arrays_of_arrays: enable
>         +
>         +attribute vec4 vert;
>         +
>         +void foo(vec4 x[2][3]);
>         +
>         +void main()
>         +{
> 
> 
> Same comment applies here.
> 
>  
>         +  gl_Position = vert;
>         +}
> 
> 
> Can we also add a test to verify that the following works:
> 
> void foo(vec4[3][2] x);
> 
> 
> (this is explicitly allowed by the spec).
> 
> 
> A similar comment goes for all the other tests in this patch.

ok will do.


> 
> As a side note, are you planning to add other tests in subsequent
> patches?  Glancing at the spec, it seems like there's a lot more that
> still needs to be tested.

Yes I plan to send patches to cover more scenarios. I have been playing
around with Mesa trying to implement the extension when time permits and
have had these tests sitting around for a while. I thought I would
submit what I had to get some feedback before continuing work on more as
it seems I will have to create a fairly large amount of tests, I'm glad
I did as it would have been a pain to have to go back retrospectively
and rename them all, but I'm sorry if I have wasted your time this was
not my intention. I will try submit the remaining tests as a series for
the compiler tests rather than sending in bits and pieces.

Thanks for the feedback.

Tim