Ian Romanick <[email protected]> writes: > On 08/26/2013 03:32 PM, Eric Anholt wrote: >> Ian Romanick <[email protected]> writes: >> >>> From: Ian Romanick <[email protected]> >>> >>> Signed-off-by: Ian Romanick <[email protected]> >>> --- >>> 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 <[email protected]>
pgpRLkpV_Qhtb.pgp
Description: PGP signature
_______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
