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

[Francisco Jerez]

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.
---
 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);
-   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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

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

[Francisco Jerez]

Jordan Justen jordan.l.jus...@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.

 -Jordan

 -   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@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/piglit


pgpGzP6lwEGWR.pgp
Description: PGP signature
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

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

[Matt Turner]

On Sat, Jan 31, 2015 at 7:41 AM, Francisco Jerez curroje...@riseup.net 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.

Do we want this function because we expect certain compilations to
fail (and if so, why are we trying to compile them?) or just for ease
of developing a large containing many subtests?

I looked at its use in 3/23 and wasn't sure of the answer.
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

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

[Jordan Justen]

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?

 ---
  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?

-Jordan

 -   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@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/piglit
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

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

[Francisco Jerez]

Hi Matt,

Matt Turner matts...@gmail.com writes:

 On Sat, Jan 31, 2015 at 7:41 AM, Francisco Jerez curroje...@riseup.net 
 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.

 Do we want this function because we expect certain compilations to
 fail (and if so, why are we trying to compile them?) or just for ease
 of developing a large containing many subtests?

This series only uses it for the second reason, i.e. to avoid killing
the program when a shader compilation fails so it can go on and run the
remaining subtests.  But I can imagine it could also be useful someday
for the first reason.

 I looked at its use in 3/23 and wasn't sure of the answer.


pgpbAeTumlIcn.pgp
Description: PGP signature
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

Re: [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.jus...@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.jus...@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@lists.freedesktop.org
  http://lists.freedesktop.org/mailman/listinfo/piglit
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit