[Piglit] [PATCH V2] util: Add multiple shader version of piglit_build_simple_program functions
Ian Romanick
idr at freedesktop.org
Wed Sep 11 07:10:17 PDT 2013
On 09/10/2013 07:12 PM, Chad Versace wrote:
> This looks mostly right. I have a few suggestions to make the code
> easier to read, and to remove some redundant code, and fix one risky bug.
>
> Also, if someone reviewed v1 of a patch, it's a really good idea
> to add him to the cc list on v2. So I've CC'd Ian for you.
>
> On 09/04/2013 04:45 PM, Jacob Penner wrote:
>> Add piglit_link_simple_program_multiple_shader()
>> Add piglit_build_simple_program_multiple_shader()
>> Add piglit_build_simple_program_unlinked_multiple_shader()
>> ---
>> tests/util/piglit-shader.c | 126
>> +++++++++++++++++++++++++++++++++++++++++++++
>> tests/util/piglit-shader.h | 7 +++
>> 2 files changed, 133 insertions(+)
>>
>> diff --git a/tests/util/piglit-shader.c b/tests/util/piglit-shader.c
>> index f7bd06f..2635a90 100644
>> --- a/tests/util/piglit-shader.c
>> +++ b/tests/util/piglit-shader.c
>> @@ -322,3 +322,129 @@ piglit_build_simple_program(const char
>> *vs_source, const char *fs_source)
>>
>> return prog;
>> }
>> +
>> +GLint piglit_link_simple_program_multiple_shaders(GLint shader1, ...)
>
> This function looks good to me, except for one small style quibble...
>
>> +{
>> + va_list ap;
>> + GLint prog, sh;
>> +
>> + piglit_require_GLSL();
>> +
>> + prog = glCreateProgram();
>> +
>> + va_start(ap, shader1);
>> + sh = shader1;
>> +
>> + while(sh != 0) {
>
> Piglit tries to follow the Linux kernel code style, which
> uses ``while (cond)``, not ``while(cond)``.
>
>> + glAttachShader(prog, sh);
>> + sh = va_arg(ap, GLint);
>> + }
>> +
>> + va_end(ap);
>> +
>> + /* If the shaders reference piglit_vertex or piglit_tex, bind
>> + * them to some fixed attribute locations so they can be used
>> + * with piglit_draw_rect_tex() in GLES.
>> + */
>> + glBindAttribLocation(prog, PIGLIT_ATTRIB_POS, "piglit_vertex");
>> + glBindAttribLocation(prog, PIGLIT_ATTRIB_TEX, "piglit_texcoord");
>> +
>> + glLinkProgram(prog);
>> +
>> + if (!piglit_link_check_status(prog)) {
>> + glDeleteProgram(prog);
>> + prog = 0;
>> + }
>> +
>> + return prog;
>> +}
>> +
>> +/**
>> + * Builds and links a program from optional sources, but does not link
>> + * it. Requires to pass 0 as the final argument. If there is a
>> compile failure,
>> + * the test is terminated.
>> + */
>
> The phrase "requires to pass 0" feels very awkward. It's also ambiguous
> how to
> exactly terminate the argument list, since the list consists of pairs
> rather
> than individuals. Please rephrase it as "The argument list's last member
> must
> be targetN=0", or something similar.
glXChooseVisual has a similar sort of interface. Its man page says:
attribList
Specifies a list of boolean attributes and integer
attribute/value
pairs. The last attribute must be None.
I think saying "The last target must be 0." should work. The man page
also includes and example, so that may be good too.
>> +GLint
>> +piglit_build_simple_program_unlinked_multiple_shader(GLenum target1,
>> + const char *source1,
>> + ...)
>> +{
>> + va_list ap;
>> + GLuint prog;
>> + const char *sh;
>> + GLenum t;
>
> Variables 'sh' is badly named. Since it iterates over source1, source2,
> etc,
> it should be renamed to 'source'.
>
> Variable 't' could be named better. So much is happening in this function
> that its name should better reflect what it does. Please rename it
> 'target'.
Yes, please.
>> +
>> + piglit_require_GLSL();
>> + prog = glCreateProgram();
>> +
>> + va_start(ap, source1);
>> +
>> + sh = source1;
>> + t = target1;
>> +
>> + while(t != 0) {
>> + GLuint shader = piglit_compile_shader_text(t, sh);
>> +
>> + glAttachShader(prog, shader);
>> + glDeleteShader(shader);
>> +
>> + t = va_arg(ap, GLenum);
>> + sh = va_arg(ap, char*);
>> + }
>
> The last line of this loop calls va_arg(ap, ...) after ap has walked
> past the
> end of the argument list. I doubt that's safe, so let's not take the risk.
> This should fix it:
>
> target = target1;
> source = source1;
>
> if (target == 0 || source == NULL)
> return prog;
>
> while (true)
> GLuint shader = piglit_compile_shader_text(target, source);
>
> glAttachShader(prog, shader);
> glDeleteShader(shader);
>
> target = va_arg(ap, GLenum);
> if (target == 0)
> break;
>
> source = va_arg(ap, char*);
> if (source == NULL)
> break;
> }
That's a good point. I think we could keep it more like Jacob's
original code by:
while (target != 0) {
GLuint shader = piglit_compile_shader_text(target, source);
glAttachShader(prog, shader);
glDeleteShader(shader);
target = va_arg(ap, GLenum);
if (target != 0)
source = va_arg(ap, char*);
}
>> +
>> + va_end(ap);
>> +
>> + return prog;
>> +}
>> +
>> +/**
>> + * Builds and links a program from optional sources, requires
>> + * to pass 0 as the final argument, throwing PIGLIT_FAIL on error.
>> + */
>
> Again, "requires to pass 0" feels awkward.
>
>> +GLint
>> +piglit_build_simple_program_multiple_shader(GLenum target1,
>> + const char *source1,
>> + ...)
>> +{
>> + va_list ap;
>> + GLuint prog;
>> + const char *sh;
>> + GLenum t;
>> +
>> + piglit_require_GLSL();
>> + prog = glCreateProgram();
>> +
>> + va_start(ap, source1);
>> +
>> + sh = source1;
>> + t = target1;
>> +
>> + while(t != 0) {
>> + GLuint shader = piglit_compile_shader_text(t, sh);
>> +
>> + glAttachShader(prog, shader);
>> + glDeleteShader(shader);
>> +
>> + t = va_arg(ap, GLenum);
>> + sh = va_arg(ap, char*);
>> + }
>> +
>> + va_end(ap);
>
> Up to this line, this function's body is identical to the body of
> piglit_build_simple_program_unlinked_multiple_shader. Don't duplicate
> that code. Instead, you should create a shared helper function that
> takes an va_list as a parameter.
Yes, please.
> Passing around variable argument lists can be tricky, so here's the
> best way to do it.
>
> ---- -8<- ----
>
> static GLuint
> piglit_build_simple_program_unlinked_multiple_shader_v(GLenum target1,
> const char *source1,
> va_list ap)
> {
> // Move the body of this patch's
> piglit_build_simple_program_unlinked_multiple_shader
> // into this function. Do not call va_start or va_end, though, from
> this function.
> // It's the caller's responsibility to initialize and teardown the
> va_list.
> }
>
> GLint
> piglit_build_simple_program_unlinked_multiple_shader(GLenum target1,
> const char *source1,
> ...)
> {
> va_list ap;
> GLuint prog;
>
> va_start(ap, source1);
> prog =
> piglit_build_simple_program_unlinked_multiple_shader_v(target1, source1,
> ap);
> va_end(ap);
> return prog;
> }
>
> GLint
> piglit_build_simple_program_multiple_shader(GLenum target1,
> const char *source1,
> ...)
> {
> va_list ap;
> GLuint prog;
>
> va_start(ap, source1);
> prog =
> piglit_build_simple_program_unlinked_multiple_shader_v(target1, source1,
> ap);
> va_end(ap);
>
> // Here do the tail end of your original
> piglit_build_simple_program_multiple_shader.
> return prog;
> }
>
> ---- ->8- ----
>
> For some *really* common examples of functions that take va_list arguments,
> see the manpage of vprintf. It's equivalent ot printf, but the '...' is
> replaced with 'va_list ap'. That manpage also contains other va_list
> printf variants.
>
>> +
>> + /* If the shaders reference piglit_vertex or piglit_tex, bind
>> + * them to some fixed attribute locations so they can be used
>> + * with piglit_draw_rect_tex() in GLES.
>> + */
>> + glBindAttribLocation(prog, PIGLIT_ATTRIB_POS, "piglit_vertex");
>> + glBindAttribLocation(prog, PIGLIT_ATTRIB_TEX, "piglit_texcoord");
>> +
>> + glLinkProgram(prog);
>> +
>> + if (!piglit_link_check_status(prog)) {
>> + glDeleteProgram(prog);
>> + prog = 0;
>> + piglit_report_result(PIGLIT_FAIL);
>> + }
>> +
>> + return prog;
>> +}
>> diff --git a/tests/util/piglit-shader.h b/tests/util/piglit-shader.h
>> index 8f18f0a..5b7c0cd 100644
>> --- a/tests/util/piglit-shader.h
>> +++ b/tests/util/piglit-shader.h
>> @@ -38,6 +38,13 @@ GLint piglit_link_simple_program(GLint vs, GLint fs);
>> GLint piglit_build_simple_program(const char *vs_source, const char
>> *fs_source);
>> GLuint piglit_build_simple_program_unlinked(const char *vs_source,
>> const char *fs_source);
>> +GLint piglit_link_simple_program_multiple_shaders(GLint shader1, ...);
>> +GLint piglit_build_simple_program_unlinked_multiple_shader(GLenum
>> target1,
>> + const char *source1,
>> + ...);
>> +GLint piglit_build_simple_program_multiple_shader(GLenum target1,
>> + const char *source1,
>> + ...);
>>
>> #if defined(PIGLIT_USE_OPENGL_ES1)
>> #define glAttachShader assert(!"glAttachShader does not exist in ES1")
>>
More information about the Piglit
mailing list