On Oct 16, 2015 01:02, "Eduardo Lima Mitev" <[email protected]> wrote: > > 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? >
I believing the readme (or is it hacking?) file specifies the style as 8 space tabs. > Thanks! > > Eduardo > _______________________________________________ > Piglit mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/piglit
_______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
