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

[Juliet Fru]

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 <emil.l.velikov at gmail.com>
wrote:

> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20150827/6828ef00/attachment.html>