<p dir="ltr"><br>
On Oct 16, 2015 01:02, "Eduardo Lima Mitev" <<a href="mailto:elima@igalia.com">elima@igalia.com</a>> wrote:<br>
><br>
> Hi Emil,<br>
><br>
> I will update the test to address all your suggestions. I left some<br>
> comments inline.<br>
><br>
> On 10/15/2015 07:25 PM, Emil Velikov wrote:<br>
> > Hi Eduardo,<br>
> ><br>
> > A few humble suggestions.<br>
> ><br>
> > On 15 October 2015 at 02:50, Eduardo Lima Mitev <<a href="mailto:elima@igalia.com">elima@igalia.com</a>> wrote:<br>
> >> This is a new test that checks valid and invalid combinations of GL_BGRA_EXT<br>
> >> format and internal format in Tex(Sub)Image2D calls, as specified by the<br>
> >> EXT_texture_format_BGRA8888 extension<br>
> >> <<a href="https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_format_BGRA8888.txt">https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_format_BGRA8888.txt</a>>.<br>
> > There is no need to provide a link to the spec in commit messages.<br>
> ><br>
> > [snip]<br>
> >> +static GLboolean<br>
> > Please use bool.<br>
> ><br>
><br>
> Ok.<br>
><br>
> >> +run_test(void)<br>
> >> +{<br>
> >> +   GLuint tex;<br>
> >> +<br>
> >> +   glGenTextures(1, &tex);<br>
> >> +   glBindTexture(GL_TEXTURE_2D, tex);<br>
> >> +   if (!piglit_check_gl_error(GL_NO_ERROR))<br>
> >> +      return false;<br>
> >> +<br>
> >> +   /* glTexImage2D */<br>
> >> +   glTexImage2D(GL_TEXTURE_2D, 0, GL_BGRA_EXT, 2, 2, 0, GL_BGRA_EXT, GL_UNSIGNED_BYTE, NULL);<br>
> > Wrap the last three arguments to the next line ? Here and below.<br>
> ><br>
> >> +   if (!piglit_check_gl_error(GL_NO_ERROR))<br>
> >> +      return false;<br>
> > Most places in piglit continue running all (sub)tests, even if there<br>
> > is a failure midway. One common pattern is<br>
> ><br>
> > bool pass = true;<br>
> ><br>
><br>
> Yes, this is common practice in tests. A foolish mistake.<br>
><br>
> > ...<br>
> ><br>
> > glFoo()<br>
> > if (!piglit_check_gl_error...)<br>
> >    pass = false;<br>
> ><br>
> > ...<br>
> > return pass;<br>
> ><br>
> ><br>
> > Cheers,<br>
> > Emil<br>
> ><br>
> > P.S. piglit tries to use tabs for indentation, albeit some tests use spaces.<br>
> ><br>
><br>
> About indentation, I see that newly added files seem to have freedom<br>
> choosing between a tab and 3-space (like Mesa). So I chose the latter<br>
> for consistency with Mesa. Do you think I should use tab for indentation<br>
> instead?<br>
></p>
<p dir="ltr">I believing the readme (or is it hacking?) file specifies the style as 8 space tabs.</p>
<p dir="ltr">> Thanks!<br>
><br>
> Eduardo<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">http://lists.freedesktop.org/mailman/listinfo/piglit</a><br>
</p>