[Mesa-dev] [PATCH 16/26] glsl: Clean up shading language mixing check for GLSL 3.00 ES.

[Kenneth Graunke]

On 11/30/2012 10:07 AM, Ian Romanick wrote:
> From: Paul Berry <stereotype441 at gmail.com>
>
> Previously, we prohibited mixing of shading language versions if
> min_version == 100 or max_version >= 130.  This was technically
> correct (since desktop GLSL 1.30 and beyond prohibit mixing of shading
> language versions, as does GLSL 1.00 ES), but it was confusing.  Also,
> we asserted that all shading language versions were between 1.00 and
> 1.40, which was unnecessary (since the parser already checks shading
> language versions) and doesn't work for GLSL 3.00 ES.
>
> This patch changes the code to explicitly check that (a) ES shaders
> aren't mixed with desktop shaders, (b) shaders aren't mixed between ES
> versions, and (c) shaders aren't mixed between desktop GLSL versions
> when at least one shader is GLSL 1.30 or greater.  Also, it removes
> the unnecessary assertion.
>
> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
> ---
>   src/glsl/linker.cpp | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index 3b2ab96..1bae043 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -2421,9 +2421,19 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>
>      unsigned min_version = UINT_MAX;
>      unsigned max_version = 0;
> +   bool is_es_prog = false;
>      for (unsigned i = 0; i < prog->NumShaders; i++) {
>         min_version = MIN2(min_version, prog->Shaders[i]->Version);
>         max_version = MAX2(max_version, prog->Shaders[i]->Version);
> +      if (i == 0) {
> +         is_es_prog = prog->Shaders[i]->IsEsShader;
> +      } else {

I really dislike loops that contain conditional code for particular 
indices.  It almost seems like you want:

bool is_es_prog = prog->Shaders[0]->IsEsShader;

then you can just omit this code within the loop.  Nice and clean.  The 
only trouble is that doing so assumes there is at least one shader.

I *believe* that calling glLinkProgram with no attached shaders is an 
error and should be rejected (oglconform claims it's invalid).  However, 
I can't find any spec text about it and I'm fairly sure we don't verify 
that NumShaders >= 1 when linking.

But it's not a big deal...just a pet peeve I picked up after reading 
code in another project which was essentially:
for (int i = 0; i < POUND_DEFINE_THAT_HAPPENS_TO_BE_THREE; i++) {
    if (i == 0) {...} else if (i == 1) {...} else if (i == 2) {...}
    /* seriously, why the hell did you make this a loop? */
}

Your code here is sensible and gets a R-b either way.

> +         if (prog->Shaders[i]->IsEsShader != is_es_prog) {
> +            linker_error(prog, "all shaders must use same shading "
> +                         "language version\n");
> +            goto done;
> +         }
> +      }
>
>         switch (prog->Shaders[i]->Type) {
>         case GL_VERTEX_SHADER:
> @@ -2444,10 +2454,10 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>      /* Previous to GLSL version 1.30, different compilation units could mix and
>       * match shading language versions.  With GLSL 1.30 and later, the versions
>       * of all shaders must match.
> +    *
> +    * GLSL ES has never allowed mixing of shading language versions.
>       */
> -   assert(min_version >= 100);
> -   assert(max_version <= 140);
> -   if ((max_version >= 130 || min_version == 100)
> +   if ((is_es_prog || max_version >= 130)
>          && min_version != max_version) {
>         linker_error(prog, "all shaders must use same shading "
>   		   "language version\n");
>