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

[Eric Anholt]

Ian Romanick <idr at freedesktop.org> writes:

> 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 
> right... and even more annoying to read / review.  What I wanted was 
> ralloc_asprintf_append.

Yeah, I stopped just short of suggesting we import ralloc :)

Love the changes.  This is all:

Reviewed-by: Eric Anholt <eric at anholt.net>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20130827/6da98fee/attachment.pgp>