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