On 25 August 2012 01:12, Chris Forbes <<a href="mailto:chrisf@ijw.co.nz" target="_blank">chrisf@ijw.co.nz</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Signed-off-by: Chris Forbes <<a href="mailto:chrisf@ijw.co.nz">chrisf@ijw.co.nz</a>> </blockquote><div> Functionally I'm fine with this patch. But it's hard to follow the diff because the patch contains both whitespace changes (fixing up indentation) and non-whitespace changes (changing the if test to a call to piglit_require_gl_version()). I would recommend separating into two patches so that the diffs are easier to follow. Also, it would be nice to put the indentation fixup before patch 2/6, so that when patch 2/6 modifies the same file, it doesn't appear to be introducing indentation problems. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> --- tests/shaders/glsl-arb-fragment-coord-conventions.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/tests/shaders/glsl-arb-fragment-coord-conventions.c b/tests/shaders/glsl-arb-fragment-coord-conventions.c index 3dbb81f..fd1a683 100644 --- a/tests/shaders/glsl-arb-fragment-coord-conventions.c +++ b/tests/shaders/glsl-arb-fragment-coord-conventions.c @@ -217,19 +217,14 @@ static void key_func(unsigned char key, int x, int y) piglit_escape_exit_key(key, x, y); } -void -piglit_init(int argc, char **argv) +void piglit_init(int argc, char **argv) { - if (piglit_get_gl_version() < 20) { - printf("Requires OpenGL 2.0\n"); - piglit_report_result(PIGLIT_SKIP); - } - - piglit_require_extension("GL_ARB_fragment_coord_conventions"); + piglit_require_gl_version(20); - if (!piglit_automatic) { - printf("Press t to switch between subtests.\n"); - glutKeyboardFunc(key_func); - } + piglit_require_extension("GL_ARB_fragment_coord_conventions"); + if (!piglit_automatic) { + printf("Press t to switch between subtests.\n"); + glutKeyboardFunc(key_func); + } } -- 1.7.12 _______________________________________________ Piglit mailing list <a href="mailto:Piglit@lists.freedesktop.org">Piglit@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/piglit" target="_blank">http://lists.freedesktop.org/mailman/listinfo/piglit</a> </blockquote></div>