On 10/07/2013 11:02 AM, Chad Versace wrote: > On 10/04/2013 06:11 PM, Ian Romanick wrote: >> From: Ian Romanick <ian.d.roman...@intel.com> >> >> This required some ugly hacking about to get the list of subtests to >> process_args. Suggestions? > > Let's avoid these ugly hacks. Here's my suggestion, which covers patches > 2 and 3.
My troll bait worked. :) > Patch 2/4 moves `struct piglit_gl_test_config config` out of main() to > file scope. Let's avoid making things global; that leads to a slippery > slope. I fought hard to kill many of Piglit's globals, and I don't welcome > the arrival of new ones. If at all possible, let's leave it in main() > where it is. > > To leave 'config' in main(), we need to restructure a few things. First, > piglit_gl_test_config_init() should no longer parse args; instead, it > should only > memset 'config' to 0. That is, let's change its signature from > > void piglit_gl_test_config_init(int *argc, char *argv[], struct > piglit_gl_test_config *config) > > to > > void piglit_gl_test_config_init(struct piglit_gl_test_config *config) > > Move the argparsing (that is, the call to process_args()) into > piglit_gl_test_run(). That will ensure > that args are parsed *after* the test author has set all config > variables in the CONFIG block. > > The CONFIG block in your patch 4/4 should now look like this: > > PIGLIT_GL_TEST_CONFIG_BEGIN > > config.subtests = subtests; > config.supports_gl_compat_version = 10; > config.window_visual = PIGLIT_GL_VISUAL_RGB; > > PIGLIT_GL_TEST_CONFIG_END I like this. This is the way I originally wanted to structure things, but there were issues. > The config is no longer defined with file-scope, so piglit_init() should > execute subtests > like this: > > result = piglit_run_subtests(PIGLIT_SKIP); > > piglit_run_selected_subtests() should access `config->subtests` through > the global > "test context object", piglit-framework-gl.c:`struct piglit_gl_framework > *gl_fw`, > like this: > > enum piglit_result > piglit_run_selected_subtests(enum piglit_result previous_result) > { > enum piglit_result result = previous_result; > const struct piglit_gl_subtest *subtests gl_fw->config->subtests; > const char **selected_subtests = gl_fw->config->selected_subtests; > > ... > } I have somewhat mixed feelings about this... I liked that the old piglit_run_selected_subtests was both more primitive (flexible) and more explicit. This is the birth of this tool, and I'm not sure how the next user will want to use it. Hmm... A third option is an accessor that lets the test get config or maybe just the list of selected tests from the implicit test context object. > At this point, it's no longer necessary for the subtests array to be > global, > so you could even do the below if you wanted, but that's really up to > personal > taste. > > PIGLIT_GL_TEST_CONFIG_BEGIN > > config.supports_gl_compat_version = 10; > config.window_visual = PIGLIT_GL_VISUAL_RGB; > > config.subtests = { > { > "Cannot delete while active", > "delete-inactive-only", > delete_inactive_only > }, > ..., > }; > > PIGLIT_GL_TEST_CONFIG_END I think that's C99 syntax, so we probably can't use it in piglit. _______________________________________________ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit