[Piglit] [PATCH 09/12] glx_arb_create_context: Verify rejection of forward-compatible flag w/pre-3.0

[Chad Versace]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/21/2011 10:35 AM, Ian Romanick wrote:
> On 12/20/2011 03:23 PM, Chad Versace wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> On 12/14/2011 10:47 AM, Ian Romanick wrote:
>>> From: Ian Romanick<ian.d.romanick at intel.com>
>>>
>>> NVIDIA's closed-source driver fails this test because it generates the
>>> wrong X error (BadAlloc instead of BadMatch).  It correctly does not
>>> create the context.
>>>
>>> AMD's closed-source driver fails this test because it does not
>>> generate any X error.  It correctly does not create the context.
>>>
>>> Signed-off-by: Ian Romanick<ian.d.romanick at intel.com>
>>> ---
>>>   tests/all.tests                                    |    1 +
>>>   .../spec/glx_arb_create_context/CMakeLists.gl.txt  |    1 +
>>>   .../invalid-flag-forward-compatible.c              |  143 ++++++++++++++++++++
>>>   3 files changed, 145 insertions(+), 0 deletions(-)
>>>   create mode 100644 tests/spec/glx_arb_create_context/invalid-flag-forward-compatible.c
>>>
>>
>>
>>
>>> +static bool try_version(int major, int minor)
>>
>> Please document try_version(). Try to do what with version? Return true
>> if context creation succeeds or fails?
>>
>> It's obvious once you read the function body. But a comment would have taken 2
>> seconds read, rather than 30 for the whole function.
> 
> Fair point.
> 
>>> +    static const struct {
>>> +        int major;
>>> +        int minor;
>>> +        bool must_pass;
>>> +    } all_gl_versions[] = {
>>> +        { 1, 0, true },
>>> +        { 1, 1, true },
>>> +        { 1, 2, true },
>>> +        { 1, 3, false },
>>> +        { 1, 4, false },
>>> +        { 1, 5, false },
>>> +        { 2, 0, false },
>>> +        { 2, 1, false }
>>
>> This array is immensely confusing for two reasons, both of which could be solved by comments.
>>
>> 1. The name 'must_pass' is misleading, and could be interpreted two ways: that context creation
>>     must pass (succeed), or that try_version() must pass. Unfortunately, I assumed it was the former,
>>     which led to much head scratching. Please document that 'must_pass' means the latter.
> 
> How about if I change the field name to 'version_must_be_supported' instead?
> 
>> 2. Why is must_pass set only for versions 1.0 - 1.2? Do we not care about the other versions?
>>     I know the answer, but I don't expect the next person to encounter this code to.
> 
> The Linux OpenGL ABI only requires OpenGL 1.2.  We want to test the other versions, but not all driver support them.  For example, i830, r100, and a bunch of older NVIDIA cards supported by Mesa only support various 1.x versions.
> 
> Does this patch to the test seem good (apologies for the crappy word-wrapping)?
> 
> diff --git a/tests/spec/glx_arb_create_context/invalid-flag-forward-compatible.c b/tests/spec/glx_arb_create_context/invalid-flag-forward-compatible.c
> index 8017ac9..1da602f 100644
> --- a/tests/spec/glx_arb_create_context/invalid-flag-forward-compatible.c
> +++ b/tests/spec/glx_arb_create_context/invalid-flag-forward-compatible.c
> @@ -41,6 +41,16 @@ static bool check_version(int major, int minor)
>         return false;
>  }
> 
> +/**
> + * Try to create a context with the GLX_CONTEXT_FORWARD_COMPATIBLE_BIT_ARB
> + *
> + * Versions previous to OpenGL 3.0 should always reject the
> + * GLX_CONTEXT_FORWARD_COMPATIBLE_BIT_ARB as it is non-sense.
> + *
> + * \return
> + * If the context is (correctly) not created, \c true is returned.  If the
> + * context is (incorrectly) created, \c false is returned.
> + */
>  static bool try_version(int major, int minor)
>  {
>         const int attribs[] = {
> @@ -87,7 +97,9 @@ int main(int argc, char **argv)
>         static const struct {
>                 int major;
>                 int minor;
> -               bool must_pass;
> +
> +               /** Linux OpenGL ABI only requires OpenGL 1.2. */
> +               bool version_must_be_supported;
>         } all_gl_versions[] = {
>                 { 1, 0, true },
>                 { 1, 1, true },
> @@ -120,7 +132,7 @@ int main(int argc, char **argv)
>          *       - Color index rendering and major version >= 3"
>          */
>         for (i = 0; i < ARRAY_SIZE(all_gl_versions); i++) {
> -               if (!all_gl_versions[i].must_pass
> +               if (!all_gl_versions[i].version_must_be_supported
>                     && !check_version(all_gl_versions[i].major,
>                                       all_gl_versions[i].minor)) {
>                         printf("OpenGL version %d.%d not supported by this "
> 

Thanks for making the test more clear. The diff looks good to me.
Reviewed-by: Chad Versace <chad.versace at linux.intel.com>

- ----
Chad Versace
chad.versace at linux.intel.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJO8ijTAAoJEAIvNt057x8ic2oQAMHgTyLRgnYO4FZ4nJfhoVx5
fszK1uaG8A/G8wdCjH2+3xfNVhS5thvq/uMRZ/kCzttdAS7ufyauDMHOYjWcWVR+
iiGNe/ayMRwonW+b6ATYDnzI3VzMo+QkT2gJWcvcBuGVcrjDefj3gdv1+GVTtiou
BfgKeKke+IADYxfIO7PAR/xoTOFlNyaiSVeBmyXONFWltmnkRRkzp1EKK6II4EiP
gOwz26Qdeus0FShkLlOgQprFKN4vnpmG1mBWD9UqQseqTSL32hZipPXD5nSYvg/r
Nlj8RJT9nPbzSWgSIJ6VECWQVIB3lgQF1gN+b4uRwHYt5FuCvjEQnLbibQrCOup8
l63AcWmTeeWG3NXLCR3Jh/uTprqhwPcy1deMnOEYB4qb67mwwVsid7Am24kAKx8s
Xd4zY6D/sKOKErTdU42bxfniwfuiYaAAuqTwEJsESn1R53MMoJ4A0GRmxODtQcXk
5HCjV4a5mtVIpDWGSSiT8Xcchw2OGwMCXsisf19pIJTwWwXUluNP1pbOobjXwx9G
Oo8jq+nzvuSuZXrDTuRhFijwdhmwqicRxZplIKc3rg8P6Rpw0cWMTBXoJ9xOVI+d
ezPn1IFTzVxAjsQkO6TbAqQMUVH53MQlReArpUp0q/5o5TzMpKR/r6aZrHgbeX+k
Ge2JbM1cPM7CdrFdzl/z
=CiBt
-----END PGP SIGNATURE-----