On 09/29/2011 11:00 PM, Carl Worth wrote: > Eric recentl mentioned to me that when setting the following > environment variables with current master mesa: > > MESA_GL_VERSION_OVERRIDE=3.0 > MESA_GLSL_VERSION_OVERRIDE=130 > > that 8 piglit preprocessor tests start to fail. > > I've investigated all of these failures and am here attaching a patch > series for review that fixes 5 of them. In the testing I've done, I > don't see any changes from this patch series other than the desired > piglit fixes. But, of course, I'd be happy for any review.
For the series: Reviewed-by: Kenneth Graunke <kenn...@whitecape.org> Thanks for fixing these, Carl! > For the remaining three failures, I believe the tests themselves are > broken and should be fixed. These fall into two case: > > 1. feature-macros/gl_gragment_precision_high.{frag,vert} > > In these tests, the actual preprocessor functionality being tested, > (that a directive of #version 130 causes the > GL_FRAGMENT_PRECISION_HIGH macro to be pre-defined), is working fine. > > The tests are failing due to the following code: > > float f() { return 0; } > > Our implementation does not currently do implicit promotion here, > so compilation fails with a type error: > > error: `return' with wrong type int, in function `f' returning > float > > The GLSL specification is not explicit on whether this promotion > should be happening, but other implementations appear to do it. So > perhaps the promotion should be added to our implementation. Odd. I could've sworn that was allowed (it would make sense), but you're right: the spec never explicitly allows that, so strictly, it's forbidden. GLSL 4.20 and the GL_ARB_shading_language_420pack extension explicitly add this functionality to the shading language, so we can add it then. I guess we could technically add GL_ARB_shading_language_420pack support now, since it only requires GLSL 1.30...it's essentially an extension that backports some new front-end functionality and clarifications to older versions of GLSL. In any case, I'm in favor of changing the test to 0.0 instead of 0. > Meanwhile, the test could legitimately be fixed to remove the type > error which is a side issue from what the test is actually trying > to do. > > 2. if/if-arg-must-be-defined-02.frag > > This test is trying to elicit an "undefined macro" error with > something like the following code: > > #if 1 > ... > #elif UNDEFINED_MACRO > ... > #endif > > I propose changing the test to use "#if 0" instead, in which case > the current implementation would pass. It's not actually desirable > to generate an error in the above case. Consider a use of a > possibly undefined macro: I agree with your analysis here. I am in favor of changing the test to "#if 0", as you suggest. > #if PERHAPS_UNDEFINED < 5 > ... > #endif > > In order to protect this use, the user might add something like the > following (or similar with #ifndef): > > #if ! defined PERHAPS_UNDEFINED > ... deal with it ... > #elif PERHAPS_UNDEFINED > ... > #endif > > In this case, the user's expectation is to get no error with this > code. Yet this is precisly the formulation the current test case > uses to elicit the error. > > I'll be glad to push changes to piglit to fix these cases unless > someone can argue aginst that. Nope, sounds good to me. Thanks! > Thanks, > > -Carl _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev