[Mesa-dev] [PATCH v2 2/3] mesa: meta: use consistent `if-else' statement throughout the function.

[Kenneth Graunke]

On 10/04/2012 04:21 AM, Oliver McFadden wrote:
> Note the blocks of the `if-else' statement are swapped; the functional
> change is only inside the condition of the `if' statement.
>
> Signed-off-by: Oliver McFadden <oliver.mcfadden at linux.intel.com>
> ---
>   src/mesa/drivers/common/meta.c |   46 ++++++++++++++++++++--------------------
>   1 files changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/src/mesa/drivers/common/meta.c b/src/mesa/drivers/common/meta.c
> index eda72da..d148c88 100644
> --- a/src/mesa/drivers/common/meta.c
> +++ b/src/mesa/drivers/common/meta.c
> @@ -3074,44 +3074,44 @@ setup_glsl_generate_mipmap(struct gl_context *ctx,
>      const char *extension_mode;
>      GLuint vs, fs;
>
> -   if (ctx->API == API_OPENGLES2 || ctx->Const.GLSLVersion < 130) {
> +   if (_mesa_is_desktop_gl(ctx) && ctx->Const.GLSLVersion >= 130) {
>         vs_source =
> -         "attribute vec2 position;\n"
> -         "attribute vec3 textureCoords;\n"
> -         "varying vec3 texCoords;\n"
> +         "#version 130\n"
> +         "in vec2 position;\n"
> +         "in vec3 textureCoords;\n"
> +         "out vec3 texCoords;\n"
>            "void main()\n"
>            "{\n"
>            "   texCoords = textureCoords;\n"
>            "   gl_Position = vec4(position, 0.0, 1.0);\n"
>            "}\n";
>         fs_template =
> -         "#extension GL_EXT_texture_array : %s\n"
> +         "#version 130\n"
>            "uniform %s texSampler;\n"
> -         "varying vec3 texCoords;\n"
> +         "in vec3 texCoords;\n"
> +         "out %s out_color;\n"
> +         "\n"
>            "void main()\n"
>            "{\n"
> -         "   gl_FragColor = %s(texSampler, %s);\n"
> +         "   out_color = texture(texSampler, %s);\n"
>            "}\n";
>      } else {
>         vs_source =
> -         "#version 130\n"
> -         "in vec2 position;\n"
> -         "in vec3 textureCoords;\n"
> -         "out vec3 texCoords;\n"
> +         "attribute vec2 position;\n"
> +         "attribute vec3 textureCoords;\n"
> +         "varying vec3 texCoords;\n"
>            "void main()\n"
>            "{\n"
>            "   texCoords = textureCoords;\n"
>            "   gl_Position = vec4(position, 0.0, 1.0);\n"
>            "}\n";
>         fs_template =
> -         "#version 130\n"
> +         "#extension GL_EXT_texture_array : %s\n"
>            "uniform %s texSampler;\n"
> -         "in vec3 texCoords;\n"
> -         "out %s out_color;\n"
> -         "\n"
> +         "varying vec3 texCoords;\n"
>            "void main()\n"
>            "{\n"
> -         "   out_color = texture(texSampler, %s);\n"
> +         "   gl_FragColor = %s(texSampler, %s);\n"
>            "}\n";
>       }
>
> @@ -3143,7 +3143,12 @@ setup_glsl_generate_mipmap(struct gl_context *ctx,
>
>      mem_ctx = ralloc_context(NULL);
>
> -   if (ctx->API == API_OPENGLES2 || ctx->Const.GLSLVersion < 130) {
> +   if (_mesa_is_desktop_gl(ctx) && ctx->Const.GLSLVersion >= 130) {
> +      fs_source = ralloc_asprintf(mem_ctx, fs_template,
> +                                  sampler->type, "vec4",
> +                                  sampler->texcoords);
> +   }
> +   else {
>         extension_mode = ((target == GL_TEXTURE_1D_ARRAY) ||
>                           (target == GL_TEXTURE_2D_ARRAY)) ?
>                          "require" : "disable";
> @@ -3152,11 +3157,6 @@ setup_glsl_generate_mipmap(struct gl_context *ctx,
>                                     extension_mode, sampler->type,
>                                     sampler->func, sampler->texcoords);
>      }
> -   else {
> -      fs_source = ralloc_asprintf(mem_ctx, fs_template,
> -                                  sampler->type, "vec4",
> -                                  sampler->texcoords);
> -   }
>
>      vs = compile_shader_with_debug(ctx, GL_VERTEX_SHADER, vs_source);
>      fs = compile_shader_with_debug(ctx, GL_FRAGMENT_SHADER, fs_source);
> @@ -3175,7 +3175,7 @@ setup_glsl_generate_mipmap(struct gl_context *ctx,
>      ralloc_free(mem_ctx);
>
>      if ((_mesa_is_desktop_gl(ctx) && ctx->Const.GLSLVersion >= 130) ||
> -       _mesa_is_gles3(ctx)){
> +       _mesa_is_gles3(ctx)) {
>         vs = compile_shader_with_debug(ctx, GL_VERTEX_SHADER, vs_int_source);
>         fs = compile_shader_with_debug(ctx, GL_FRAGMENT_SHADER, fs_int_source);

I'm still ambivalent about this patch.  Before the patch, it was:

if (ES2 || < 130) {
    ...
} else {
    ...
}

if (ES2 || < 130) {
    ...
} else {
    ...
}

if ((desktop && >= 130) || gles3) {
    ...
}

The first two were already consistent, and the last one is different 
because there's no else case...it only applies in the one case.

The existing code seems fine to me.  That said, your patch seems 
correct, so I suppose it's fine as well.