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

[Matt Turner]

On Tue, Oct 1, 2013 at 3:06 PM, Chad Versace
<chad.versace at linux.intel.com> wrote:
> 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?
>    }

Looks good to me. Have a R-b. I don't see any improvements to it.