On 7 January 2014 15:25, Kenneth Graunke <kenn...@whitecape.org> wrote:
> 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.
>
No need to lecture me--I'm a fan of small patches too. It was an honest
mistake that I wrapped so many changes into this one patch. And I disagree
that it's not worth changing--splitting patches is easy, and what's the
point of patch review if we don't improve the patches based on it? I split
the patch into four:
glsl: make _mesa_shader_stage_to_string() available to non-C++ code.
mesa: Move declaration of gl_shader_stage earlier in mtypes.h.
mesa: Store gl_shader_stage enum in gl_shader objects.
mesa: Use gl_shader::Stage instead of gl_shader::Type where possible.
>
> [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)
>
True. I've made a behavioural change, which is to make this function use a
suffix of "????" rather than "geom" if it encounters a shader for an
unexpected stage. But I couldn't see a good way to split this out to its
own patch without writing throw away code that would be rewritten in the
patch that immediately followed it.
I've made a note in the commit message of the behavioural change so that it
doesn't come as a surprise to people.
>
> 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>
>
Thanks for the review!
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
