<div dir="ltr">On 17 January 2013 15:08, 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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
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>
---<br>
 tests/shaders/shader_runner.c |    5 ++++-<br>
 1 file changed, 4 insertions(+), 1 deletion(-)<br>
<br>
diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c<br>
index 8dfeb2a..6033bda 100644<br>
--- a/tests/shaders/shader_runner.c<br>
+++ b/tests/shaders/shader_runner.c<br>
@@ -869,7 +869,10 @@ parse_required_versions(struct requirement_parse_results *results,<br>
<br>
        while (line[0] != '\0') {<br>
                if (line[0] == '[')<br>
-                       in_requirement_section = false;<br>
+                       if (in_requirement_section)<br>
+                               break;<br>
+                       else<br>
+                               in_requirement_section = false;<br>
<br>
                if (!in_requirement_section) {<br>
                        if (string_match("[require]", line)) {<br>
<span class=""><font color="#888888">--<br>
1.7.10.4<br>
<br>
_______________________________________________<br>
Piglit mailing list<br>
<a href="mailto:Piglit@lists.freedesktop.org">Piglit@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/piglit" target="_blank">http://lists.freedesktop.org/mailman/listinfo/piglit</a><br>
</font></span></blockquote></div><br></div><div class="gmail_extra">I'm uncomfortable with this patch for a few reasons:<br><br></div><div class="gmail_extra">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.<br>
<br></div><div class="gmail_extra">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.<br>
</div></div>