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

[Ian Romanick]

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.