[Mesa-dev] [PATCH 2/7] mesa: Store gl_shader_stage enum in gl_shader objects.

[Kenneth Graunke]

On 01/07/2014 02:13 PM, Paul Berry wrote:
> This reduces confusion since gl_shader::Type is sometimes
> GL_SHADER_PROGRAM_MESA but is more frequently
> GL_SHADER_{VERTEX,GEOMETRY,FRAGMENT}.  It also has the advantage that
> when switching on gl_shader::Stage, the compiler will alert if one of
> the possible enum types is unhandled.  Finally, many functions in
> src/glsl (especially those dealing with linking) already use
> gl_shader_stage to represent pipeline stages; storing a
> gl_shader_stage in the gl_shader object avoids the need for a
> conversion.

If you'll permit me a bit of grumbling...

This patch does more than it says.  The clear purpose of this patch is
to add a new field.  (Which is a non-trivial amount of work - moving
definitions around, adding new code to set it in at least 4 places...)

But then it goes and does a bunch of follow-on cleaning to /use/ the new
field in a bunch of random places.  This would've been great as a second
patch...but alas.

Not worth changing at this point, but for future reference, please...if
you're going to rename something, rename that one thing.  If you're
going to add a field, add that one field.  More patches are fine.

[snip]

> diff --git a/src/mesa/program/prog_print.c b/src/mesa/program/prog_print.c
> index fa120cc..9391e99 100644
> --- a/src/mesa/program/prog_print.c
> +++ b/src/mesa/program/prog_print.c
> @@ -1005,16 +1005,21 @@ _mesa_print_parameter_list(const struct gl_program_parameter_list *list)
>  void
>  _mesa_write_shader_to_file(const struct gl_shader *shader)
>  {
> -   const char *type;
> +   const char *type = "????";

As far as I can tell this is a spurious change which is neither:
- adding the new enum to the field (supposed purpose of patch)
- changing things to use the new field directly (secondary change done
in this patch)

Other than those minor grumbles, though, I really like what you're doing
here!

As is, the series is:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

>     char filename[100];
>     FILE *f;
>  
> -   if (shader->Type == GL_FRAGMENT_SHADER)
> +   switch (shader->Stage) {
> +   case MESA_SHADER_FRAGMENT:
>        type = "frag";
> -   else if (shader->Type == GL_VERTEX_SHADER)
> +      break;
> +   case MESA_SHADER_VERTEX:
>        type = "vert";
> -   else
> +      break;
> +   case MESA_SHADER_GEOMETRY:
>        type = "geom";
> +      break;
> +   }
>  
>     _mesa_snprintf(filename, sizeof(filename), "shader_%u.%s", shader->Name, type);
>     f = fopen(filename, "w");
> @@ -1061,7 +1066,7 @@ _mesa_append_uniforms_to_file(const struct gl_shader *shader)
>     char filename[100];
>     FILE *f;
>  
> -   if (shader->Type == GL_FRAGMENT_SHADER)
> +   if (shader->Stage == MESA_SHADER_FRAGMENT)
>        type = "frag";
>     else
>        type = "vert";
> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> index b2131ed..bd4eb5e 100644
> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> @@ -5207,6 +5207,7 @@ st_new_shader(struct gl_context *ctx, GLuint name, GLuint type)
>     shader = rzalloc(NULL, struct gl_shader);
>     if (shader) {
>        shader->Type = type;
> +      shader->Stage = _mesa_shader_enum_to_shader_stage(type);
>        shader->Name = name;
>        _mesa_init_shader(ctx, shader);
>     }
>