[Piglit] [PATCH] Porting basic GL rendering paths test to piglit

[Emil Velikov]

On 25 August 2015 at 15:56, Brian Paul <brian.e.paul at gmail.com> wrote:
> Looks good overall.  Just a bunch of nit-picks below...
>
>
> On Mon, Aug 24, 2015 at 5:19 PM, Juliet Fru <julietfru at gmail.com> 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