Re: [Piglit] [PATCH 01/23] util/shader: Define "nothrow" variant of piglit_compile_shader_text().
On 2015-01-31 11:45:59, Francisco Jerez wrote:
> Jordan Justen 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
> >
> >> - 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().
Hi Matt, Matt Turner writes: > On Sat, Jan 31, 2015 at 7:41 AM, 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. > > 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 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().
On Sat, Jan 31, 2015 at 7:41 AM, 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. 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().
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
[Piglit] [PATCH 01/23] util/shader: Define "nothrow" variant of piglit_compile_shader_text().
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
