<div dir="ltr"><div><div><div>Hello Emil,<br><br></div>Thank you for the corrections you have given. I will make the recommended changes and send and updated patch to the mailing list.<br><br></div>Best Regards,<br></div>Juliet<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug 26, 2015 at 2:46 PM, Emil Velikov <span dir="ltr"><<a href="mailto:emil.l.velikov@gmail.com" target="_blank">emil.l.velikov@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 25 August 2015 at 15:56, Brian Paul <<a href="mailto:brian.e.paul@gmail.com">brian.e.paul@gmail.com</a>> wrote:<br>
> Looks good overall.  Just a bunch of nit-picks below...<br>
><br>
><br>
> On Mon, Aug 24, 2015 at 5:19 PM, Juliet Fru <<a href="mailto:julietfru@gmail.com">julietfru@gmail.com</a>> wrote:<br>
<br>
</span><span class="">>> +typedef enum {<br>
>> +       ALPHA,<br>
>> +       BLEND,<br>
>> +       COLOR_MASK,<br>
>> +       DEPTH,<br>
>> +       LOGIC,<br>
>> +       SCISSOR,<br>
>> +       STENCIL,<br>
>> +       STIPPLE,<br>
>> +       TEXTURE,<br>
>> +       ZZZ  /* end-of-list token */<br>
>> +} Path;<br>
>> +<br>
</span>There are only a couple of tests in piglit that use typedefs. How<br>
about using plain enums like below<br>
<br>
enum path/state {<br>
....<br>
}<br>
<br>
<br>
...<br>
>> +path_name(Path path) {<br>
<span class=""><br>
>> +       default:<br>
>> +               return "???";<br>
</span>You can drop the default statement here, or even change this function<br>
into a static const char * const array indexed by the enum value ?<br>
<br>
<br>
...<br>
<span class="">>> +void<br>
>> +set_path_state(Path path, State state) {<br>
<br>
</span><span class="">>> +       default:<br>
>> +               abort(); /* error */<br>
</span>Not something that common to piglit (apart from glean or tests<br>
directly ported from it)<br>
<br>
<br>
...<br>
<span class="">>> +enum piglit_result<br>
>> +piglit_display(void)<br>
>> +{<br>
>> +       bool pass = true;<br>
>> +       Path p, paths[1000];<br>
</span>Does paths really need to be that big. Afaics it will be up-to<br>
(int)(ZZZ-ALPHA) in size.<br>
<span class=""><br>
>> +       int i, numPaths = 0;<br>
>> +<br>
>> +       /* draw 10x10 pixel quads */<br>
>> +       glViewport(0, 0, 10, 10);<br>
>> +<br>
>> +       glDisable(GL_DITHER);<br>
>> +<br>
>> +       /* Build the list of paths to exercise.<br>
>> +        * Skip depth test if we have no depth buffer. */<br>
>> +       /*  Skip stencil test if we have no stencil buffer. */<br>
</span>Multi-line comments tend to be formatted line<br>
<br>
/* This is a multi-line comment.<br>
 * which spans across multiple<br>
 * lines.<br>
<span class=""> */<br>
<br>
>> +       for (p = ALPHA; p != ZZZ; p = (Path) (p + 1)) {<br>
</span>The comment is around but the actual check(s) never made it. Is that<br>
intentional ?<br>
<span class=""><br>
>> +               paths[numPaths++] = p;<br>
>> +       }<br>
<br>
<br>
</span>...<br>
<span class="">>> +<br>
>> +               set_path_state(paths[i], DISABLE);<br>
>> +<br>
>> +               /* test buffer */<br>
>> +               GLfloat pixel[3];<br>
</span>Mixing variable declarations and code will generate a build warning<br>
(and/or break the build on some platforms). Please try to avoid that<br>
when possible.<br>
<br>
<br>
...<br>
<span class="">>> +       /* success */<br>
>> +       pass = true;<br>
>> +<br>
>> +       return pass ? PIGLIT_PASS : PIGLIT_FAIL;<br>
><br>
><br>
> Remove the 'pass' var and just return PIGLIT_PASS here.<br>
><br>
</span>Alternatively, you can rework the test to run all subtests even if one<br>
or more fail :-)<br>
<br>
Cheers,<br>
Emil<br>
</blockquote></div><br></div>