[Piglit] [PATCH 1/3] egl_khr_create_context: Fix tests for invalid flags

[Chad Versace]

On 10/01/2013 02:49 PM, Matt Turner wrote:
> On Fri, Sep 20, 2013 at 7:02 PM, Chad Versace
> <chad.versace at linux.intel.com> wrote:
>> According to version 15 of the EGL_KHR_create_context spec,
>> EGL_CONTEXT_OPENGL_DEBUG_BIT_KHR is a valid flag for OpenGL ES contexts.
>> But the test "EGL_KHR_create_context/invalid flag GLES" verifies that it
>> is an *invalid* flag.
>>
>> Signed-off-by: Chad Versace <chad.versace at linux.intel.com>
>> ---
>>   .../spec/egl_khr_create_context/invalid-flag-gles.c   | 19 ++++++++++---------
>>   1 file changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/tests/egl/spec/egl_khr_create_context/invalid-flag-gles.c b/tests/egl/spec/egl_khr_create_context/invalid-flag-gles.c
>> index b12db20..5af773b 100644
>> --- a/tests/egl/spec/egl_khr_create_context/invalid-flag-gles.c
>> +++ b/tests/egl/spec/egl_khr_create_context/invalid-flag-gles.c
>> @@ -60,19 +60,20 @@ int main(int argc, char **argv)
>>          uint32_t flag = 0x80000000;
>>          bool ran_test = false;
>>
>> -       /* The EGL_KHR_create_context spec says:
>> +       /* According to the EGL_KHR_create_context spec, version 15, there
>> +        * exists exactly one valid flag for OpenGL ES contexts: the debug
>> +        * flag.
>>           *
>> -        *     "The value for attribute EGL_CONTEXT_FLAGS_KHR specifies a set of
>> -        *     flag bits affecting the context. Flags are only defined for OpenGL
>> -        *     context creation, and specifying a flags value other than zero for
>> -        *     other types of contexts, including OpenGL ES contexts, will generate
>> -        *     an error."
>> +        *     If the EGL_CONTEXT_OPENGL_DEBUG_BIT_KHR flag bit is set in
>> +        *     EGL_CONTEXT_FLAGS_KHR, then a <debug context> will be created.
>> +        *     [...] This bit is supported for OpenGL and OpenGL ES contexts.
>>           */
>> -       uint32_t first_valid_flag = 0;
>> +       const EGLint valid_flags = EGL_CONTEXT_OPENGL_DEBUG_BIT_KHR;
>> +       const EGLint invalid_flags = ~valid_flags;
>>
>>          if (EGL_KHR_create_context_setup(EGL_OPENGL_ES_BIT)) {
>>                  ran_test = true;
>> -               while (flag != first_valid_flag) {
>> +               while (flag & invalid_flags) {
>>                          pass = pass && try_flag(flag);
>>                          flag >>= 1;
>>                  }
>> @@ -83,7 +84,7 @@ int main(int argc, char **argv)
>>          if (EGL_KHR_create_context_setup(EGL_OPENGL_ES2_BIT)) {
>>                  ran_test = true;
>>                  flag = 0x80000000;
>> -               while (flag != first_valid_flag) {
>> +               while (flag & invalid_flags) {
>>                          pass = pass && try_flag(flag);
>>                          flag >>= 1;
>>                  }
>> --
>> 1.8.3.1
>
> Although the code seems a bit silly doing this while (flag !=
> first_valid_flag) when first_valid_flag is const 0, I did it that way
> to match the invalid-flag-gl.c test. I like what you're doing better
> though.
>
> This patch is
>
> Reviewed-by: Matt Turner <mattst88 at gmail.com>
>
> I'd be nice to make the invalid-flag-gl.c test behave the same way as
> this one. Maybe do a follow-on patch?

Sure, I'll do a follow-on patch.

Hmm... I now see something better than my while-loop.

    while (flag) {
       if (flag & invalid_flag) {
           pass = pass && try_flag(flag);
       }

       flag >>= 1;
    }

With this idiom, if Khronos ever adds more valid flags such that
the invalid_flags mask has discontinuities in it, then the new loop
will correctly jump over discontinuities and continue. The loop
currently in the patch would fail to test all invalid flags below
the first valid one. :( And that was exactly the behavior I was trying
to fix.

If I use the new proposed loop idiom in this patch, do I still have your
rb? Do you see any improvements that could be made?
    }