[Piglit] [PATCH] ext_texture_format_bgra8888: Add test for GL_BGRA format in Tex(Sub)Image calls

[Dylan Baker]

On Oct 16, 2015 01:02, "Eduardo Lima Mitev" <elima at igalia.com> 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 <elima at igalia.com>
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
> Piglit at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20151016/526f942f/attachment.html>