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 <kenn...@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);
> }
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
