[Piglit] [PATCH 01/23] util/shader: Define "nothrow" variant of piglit_compile_shader_text().

[Jordan Justen]

On 2015-01-31 11:45:59, Francisco Jerez wrote:
> Jordan Justen <jordan.l.justen at intel.com> writes:
> 
> > On 2015-01-31 07:41:23, Francisco Jerez wrote:
> >> Define a variant of piglit_compile_shader_text() that doesn't call
> >> piglit_report_result() on failure killing the program, which is quite
> >> annoying for tests that expect a compilation to fail and for tests
> >> that are structured in a number of subtests, because a single sub-test
> >> failing to compile a shader will prevent the remaining tests from
> >> running.
> >> 
> >> I guess this would ideally be the default behavior of
> >> piglit_compile_shader_text(), but with >300 callers in tree it seems
> >> rather difficult to change at this stage.
> >
> > sed?
> >
> > Maybe piglit_compile_shader_text => piglit_require_compile_shader_text?
> >
> I personally don't care enough to make such a change affecting hundreds
> of other tests that wouldn't have the slightest chance of being reviewed
> before it no longer applies cleanly.  I would support the change though
> if you feel like doing it.
> 
> >> ---
> >>  tests/util/piglit-shader.c | 20 ++++++++++++++++++--
> >>  tests/util/piglit-shader.h |  1 +
> >>  2 files changed, 19 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/tests/util/piglit-shader.c b/tests/util/piglit-shader.c
> >> index e8fe9c4..37cc7cc 100644
> >> --- a/tests/util/piglit-shader.c
> >> +++ b/tests/util/piglit-shader.c
> >> @@ -122,7 +122,7 @@ shader_name(GLenum target)
> >>   * Convenience function to compile a GLSL shader.
> >>   */
> >>  GLuint
> >> -piglit_compile_shader_text(GLenum target, const char *text)
> >> +piglit_compile_shader_text_nothrow(GLenum target, const char *text)
> >>  {
> >>         GLuint prog;
> >>         GLint ok;
> >> @@ -149,7 +149,8 @@ piglit_compile_shader_text(GLenum target, const char *text)
> >>                                 info);
> >>  
> >>                         fprintf(stderr, "source:\n%s", text);
> >
> > Do you think piglit_compile_shader_text_nothrow should be silent if
> > the shader fails to compile?
> >
> > Maybe move the fprintf's as well?
> >
> 
> As the main motivation for this function is to avoid killing the program
> when a shader compilation fails, I think the fprintf() is fine and
> useful to find out what is going on when something fails.  But we could
> make it dependent on a parameter or factor it out to a separate function
> if you like.

Okay. Let's go ahead with this for now.

Reviewed-by: Jordan Justen <jordan.l.justen at intel.com>

> >
> >> -                       piglit_report_result(PIGLIT_FAIL);
> >> +                       glDeleteShader(prog);
> >> +                       prog = 0;
> >>                 }
> >>                 else if (0) {
> >>                         /* Enable this to get extra compilation info.
> >> @@ -164,6 +165,21 @@ piglit_compile_shader_text(GLenum target, const char *text)
> >>         return prog;
> >>  }
> >>  
> >> +/**
> >> + * Convenience function to compile a GLSL shader.  Throws PIGLIT_FAIL
> >> + * on error terminating the program.
> >> + */
> >> +GLuint
> >> +piglit_compile_shader_text(GLenum target, const char *text)
> >> +{
> >> +        GLuint shader = piglit_compile_shader_text_nothrow(target, text);
> >> +
> >> +        if (!shader)
> >> +                piglit_report_result(PIGLIT_FAIL);
> >> +
> >> +        return shader;
> >> +}
> >> +
> >>  static GLboolean
> >>  link_check_status(GLint prog, FILE *output)
> >>  {
> >> diff --git a/tests/util/piglit-shader.h b/tests/util/piglit-shader.h
> >> index e2eef03..9208451 100644
> >> --- a/tests/util/piglit-shader.h
> >> +++ b/tests/util/piglit-shader.h
> >> @@ -31,6 +31,7 @@
> >>  void piglit_get_glsl_version(bool *es, int* major, int* minor);
> >>  
> >>  GLuint piglit_compile_shader(GLenum target, const char *filename);
> >> +GLuint piglit_compile_shader_text_nothrow(GLenum target, const char *text);
> >>  GLuint piglit_compile_shader_text(GLenum target, const char *text);
> >>  GLboolean piglit_link_check_status(GLint prog);
> >>  GLboolean piglit_link_check_status_quiet(GLint prog);
> >> -- 
> >> 2.1.3
> >> 
> >> _______________________________________________
> >> Piglit mailing list
> >> Piglit at lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/piglit