[Piglit] [PATCH 12/13] glsl: Enhance built-in-constants to test extensions too

[Ian Romanick]

On 08/26/2013 04:55 PM, Ian Romanick wrote:
> On 08/26/2013 03:32 PM, Eric Anholt wrote:
>> Ian Romanick <idr at freedesktop.org> writes:
>>
>>> From: Ian Romanick <ian.d.romanick at intel.com>
>>>
>>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>>> ---
>>>   tests/shaders/built-in-constants.c | 90
>>> +++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 89 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/shaders/built-in-constants.c
>>> b/tests/shaders/built-in-constants.c
>>> index 9e5b940..fb955ae 100644
>>> --- a/tests/shaders/built-in-constants.c
>>> +++ b/tests/shaders/built-in-constants.c
>>> @@ -39,6 +39,15 @@ unsigned num_tests = 0;
>>>   int required_glsl_version = 0;
>>>   char *required_glsl_version_string = NULL;
>>>
>>> +/**
>>> + * NUL separated list of extensions required for the current test set.
>>> + *
>>> + * The list is terminated by two consecutive NUL characters.
>>> + * \c required_extensions_length is the total size of \c
>>> required_extensions
>>> + * that has been consumed \b not \b including the NUL characters at
>>> the end.
>>> + */
>>> +char required_extensions[500] = { '\0', '\0' };
>>> +unsigned required_extensions_length = 0;
>>>
>>>   static const char *const uniform_template =
>>>       "uniform float f[%s %s %d ? 1 : -1];\n"
>>> @@ -148,6 +157,7 @@ parse_file(const char *filename)
>>>       /* The format of the test file is:
>>>        *
>>>        * major.minor
>>> +     * GL_ARB_some_extension
>>>        * gl_MaxFoo 8
>>>        * gl_MaxBar 16
>>>        * gl_MinAsdf -2
>>> @@ -169,6 +179,42 @@ parse_file(const char *filename)
>>>       if (line[0] != '\0')
>>>           line++;
>>>
>>> +    /* Process the list of required extensions.
>>> +     */
>>> +    while (strncmp("GL_", line, 3) == 0) {
>>> +        char *end_of_line = strchrnul(line, '\n');
>>> +        const ptrdiff_t len = end_of_line - line;
>>> +
>>> +        assert(end_of_line[0] == '\n' || end_of_line[0] == '\0');
>>> +
>>> +        if ((required_extensions_length + len + 2)
>>> +            > sizeof(required_extensions)) {
>>> +            fprintf(stderr, "Too many required extensions!\n");
>>> +            piglit_report_result(PIGLIT_FAIL);
>>> +        }
>>> +
>>> +        /* Copy the new extension to the list.
>>> +         */
>>> +        memcpy(&required_extensions[required_extensions_length],
>>> +               line,
>>> +               len);
>>> +
>>> +        /* Advance the count.
>>> +         */
>>> +        required_extensions_length += len;
>>> +
>>> +        /* Terminate the list.
>>> +         */
>>> +        required_extensions[required_extensions_length] = '\0';
>>> +        required_extensions[required_extensions_length + 1] = '\0';
>>> +
>>> +        /* Advance to the next input line.
>>> +         */
>>> +        line = end_of_line;
>>> +        if (line[0] == '\n')
>>> +            line++;
>>> +    }
>>
>> This code seems more clever than necessary.  If we're going to have a
>> static array of characters, how about a static array of pointers to
>> strndup()ed extension names instead?
>
> I thought that I had thought of that (lol), but I actually mis-read what
> you wrote.  I completely forgot that strndup existed.  Just using strdup
> ends up with most of the same cleverness as the current code.  I like that.
>
>> And, if there's a static array of characters here, having the
>> dynamically allocated extension enable string below with tricky checking
>> to make sure we don't free the non-malloced string seems silly, as
>> opposed to just strcat/sprintf/something.
>
> Doing the snprintf size accounting (to make sure I didn't overflow the
> static buffer) seemed like it would be even more annoying to get

Actually, it wasn't so bad.

> right... and even more annoying to read / review.  What I wanted was
> ralloc_asprintf_append.
>
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
>