<div dir="ltr">On 21 January 2013 12:03, Tom Gall <span dir="ltr"><<a href="mailto:tom.gall@linaro.org" target="_blank">tom.gall@linaro.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On Mon, Jan 21, 2013 at 12:49 PM, Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> wrote:<br>
> On 17 January 2013 15:08, Tom Gall <<a href="mailto:tom.gall@linaro.org">tom.gall@linaro.org</a>> wrote:<br>
>><br>
>> When shader_runner parses the shader_test data file, add an<br>
>> optimization such that after the [required] section is left to<br>
>> no longer parse the rest of the file.<br>
>><br>
>> Signed-off-by: Tom Gall <<a href="mailto:tom.gall@linaro.org">tom.gall@linaro.org</a>><br>
</div><snip><br>
<div class="im">> I'm uncomfortable with this patch for a few reasons:<br>
><br>
> 1. This patch is intended as a performance optimization, but we have no<br>
> evidence that there's a performance bottleneck here.  In fact, I think it's<br>
> extremely unlikely that shader_runner's parse_required_versions() function<br>
> will ever constitute a performance bottleneck, because the amount of work<br>
> being done in this function to skip the rest of the test file is miniscule<br>
> compared to the work performed by the GL implementation in running the test.<br>
> I don't think we should be making performance improvements in code that<br>
> doesn't need it unless there's some other reason we need the change (e.g.<br>
> readability, maintainability, portability, or paving the way for future<br>
> changes)--it just risks bugs.<br>
<br>
</div>Let me potentially convince you otherwise.<br>
<div class="im"><br>
> 2. This patch actually introduces a change in behaviour.  Previously, if a<br>
> test had multiple "[require]" sections, parse_required_versions() would<br>
> examine them all.  Granted, I'm not aware of any tests that have multiple<br>
> "[require]" sections (nor any reason to write one), but since it's not<br>
> prohibited, there's a decent chance someone will make one by accident one of<br>
> these days (e.g. by accidentally duplicating the "[require]" line), and if<br>
> they do, this patch will cause them to get confusing errors from<br>
> shader_runner.<br>
<br>
</div>If you have multiple [require] sections, I submit that is at worst a<br>
bug and at best asking for trouble.<br>
<br>
The last place where one sets a value such as GLSL or GL is what will<br>
win if one sets the same value more than once. What practical use is<br>
that? When one is debugging, you'll likely open the data file, see the<br>
[require] section and stop right there. There's no reason for the<br>
average tourist to think to themselves they should scroll down the<br>
file and look for another require section.<br>
<br>
Further, why would one want to specify say GL in one require section<br>
and then have another require section to specify say GLSL further down<br>
the file? This is just being obscure for obscurities sake.<br>
<br>
Last if one rewrote the tool a bit, I could see where one might have<br>
more than more than one test across different APIs jammed together,<br>
but again, what practical purpose would that have?<br>
Ex:<br>
[require]<br>
shaders..<br>
test<br>
[require]<br>
shaders<br>
another test<br>
...<br>
<br>
<br>
Allowing this behavior I think just opens the door to some ugly<br>
testcases which I can't see passing review.<br>
<br>
--<br>
Regards,<br>
Tom<br></blockquote><div><br></div><div>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.<br>
<br></div><div>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?<br>
</div></div></div></div>