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