[Piglit] [PATCH V2] util: Add multiple shader version of piglit_build_simple_program functions

[Chad Versace]

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.

> +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'.

> +
> +	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;
	}


> +
> +	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.

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")
>