On 21 January 2013 12:03, Tom Gall <tom.g...@linaro.org> wrote: > On Mon, Jan 21, 2013 at 12:49 PM, Paul Berry <stereotype...@gmail.com> > wrote: > > On 17 January 2013 15:08, Tom Gall <tom.g...@linaro.org> wrote: > >> > >> When shader_runner parses the shader_test data file, add an > >> optimization such that after the [required] section is left to > >> no longer parse the rest of the file. > >> > >> Signed-off-by: Tom Gall <tom.g...@linaro.org> > <snip> > > I'm uncomfortable with this patch for a few reasons: > > > > 1. This patch is intended as a performance optimization, but we have no > > evidence that there's a performance bottleneck here. In fact, I think > it's > > extremely unlikely that shader_runner's parse_required_versions() > function > > will ever constitute a performance bottleneck, because the amount of work > > being done in this function to skip the rest of the test file is > miniscule > > compared to the work performed by the GL implementation in running the > test. > > I don't think we should be making performance improvements in code that > > doesn't need it unless there's some other reason we need the change (e.g. > > readability, maintainability, portability, or paving the way for future > > changes)--it just risks bugs. > > Let me potentially convince you otherwise. > > > 2. This patch actually introduces a change in behaviour. Previously, if > a > > test had multiple "[require]" sections, parse_required_versions() would > > examine them all. Granted, I'm not aware of any tests that have multiple > > "[require]" sections (nor any reason to write one), but since it's not > > prohibited, there's a decent chance someone will make one by accident > one of > > these days (e.g. by accidentally duplicating the "[require]" line), and > if > > they do, this patch will cause them to get confusing errors from > > shader_runner. > > If you have multiple [require] sections, I submit that is at worst a > bug and at best asking for trouble. > > The last place where one sets a value such as GLSL or GL is what will > win if one sets the same value more than once. What practical use is > that? When one is debugging, you'll likely open the data file, see the > [require] section and stop right there. There's no reason for the > average tourist to think to themselves they should scroll down the > file and look for another require section. > > Further, why would one want to specify say GL in one require section > and then have another require section to specify say GLSL further down > the file? This is just being obscure for obscurities sake. > > Last if one rewrote the tool a bit, I could see where one might have > more than more than one test across different APIs jammed together, > but again, what practical purpose would that have? > Ex: > [require] > shaders.. > test > [require] > shaders > another test > ... > > > Allowing this behavior I think just opens the door to some ugly > testcases which I can't see passing review. > > -- > Regards, > Tom >
I agree with you 100% that having multiple "[require]" sections in a test is a bad idea. If you'd like to submit a patch that detects multiple require sections and reports an error, that would be wonderful and I'd be happy to give it my review. Once we've done that, however, I'm afraid I'm still not understanding the benefit of this patch. Do you have evidence that this patch will produce a measurable performance improvement? Or is there some reason other than a performance improvement why you think the code is better this way?
_______________________________________________ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
