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

[Ian Romanick]

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 "