<div dir="ltr">On 27 February 2013 10:59, Chad Versace <span dir="ltr"><<a href="mailto:chad.versace@linux.intel.com" target="_blank">chad.versace@linux.intel.com</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 02/27/2013 10:47 AM, Paul Berry wrote:<br>
> On 27 February 2013 10:44, Eric Anholt <<a href="mailto:eric@anholt.net">eric@anholt.net</a><br>
</div>> <mailto:<a href="mailto:eric@anholt.net">eric@anholt.net</a>>> wrote:<br>
<div><div class="h5">><br>
>     Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a> <mailto:<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>>> writes:<br>
><br>
>     > Previously, if the user specified an ill-formed GLSL version number<br>
>     > (or the implementation supplied an ill-formed number in its response<br>
>     > to glGetString(GL_SHADING_LANGUAGE_VERSION)), glslparsertest would<br>
>     > access uninitialized variables, resulting in unpredictable (and often<br>
>     > confusing) behaviour.<br>
>     ><br>
>     > With this patch, glslparser test accepts version numbers either of the<br>
>     > form "<int>" or "<int>.<int>".  Ill-formed version numbers lead to a<br>
>     > test failure.<br>
>     > ---<br>
>     >  tests/glslparsertest/glslparsertest.c | 10 +++++++---<br>
>     >  1 file changed, 7 insertions(+), 3 deletions(-)<br>
>     ><br>
>     > diff --git a/tests/glslparsertest/glslparsertest.c<br>
>     b/tests/glslparsertest/glslparsertest.c<br>
>     > index 43bef03..c9696be 100644<br>
>     > --- a/tests/glslparsertest/glslparsertest.c<br>
>     > +++ b/tests/glslparsertest/glslparsertest.c<br>
>     > @@ -339,10 +339,14 @@ int process_options(int argc, char **argv)<br>
>     >  static unsigned<br>
>     >  parse_glsl_version_number(const char *str)<br>
>     >  {<br>
>     > -     unsigned major;<br>
>     > -     unsigned minor;<br>
>     > +     unsigned major = 0;<br>
>     > +     unsigned minor = 0;<br>
>     > +<br>
>     > +     if (sscanf(str, "%u.%u", &major, &minor) == 0) {<br>
>     > +             printf("Ill-formed GLSL version number: %s\n", str);<br>
>     > +             piglit_report_result(PIGLIT_FAIL);<br>
>     > +     }<br>
><br>
>     For full pedantry, I think that would be sscanf(...) != 2.<br>
><br>
><br>
> Actually, sscanf(...) == 0 allows the version number to be supplied as either<br>
> "<int>" or "<int>.<int>".<br>
<br>
</div></div>Please add a comment explaining that's why `== 0` is used. I saw your trick,<br>
but it's not obvious.<br>
<br>
With that, the series is:<br>
Reviewed-by: Chad Versace <<a href="mailto:chad.versace@linux.intel.com">chad.versace@linux.intel.com</a>><br>
<br>
By the way, I do prefer Tom's suggestions to use runtime detection in<br>
patches 1 and 2. That would be one less thing to clean up when we switch<br>
to unified binaries. But, it doesn't really matter today.<br>
<br>
</blockquote></div><br></div><div class="gmail_extra">Ok, with both you and Tom weighing in on this, I'm convinced.  I'll go ahead and make the change before pushing the patches--it's an easy fix, and there's no sense in having needless churn.<br>
</div></div>