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

[Paul Berry]

On 7 January 2014 15:25, Kenneth Graunke <kenneth at 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 <kenneth at whitecape.org>
>

Thanks for the review!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140108/1302c14a/attachment.html>