<div dir="ltr">On 7 January 2014 15:25, Kenneth Graunke <span dir="ltr"><<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">On 01/07/2014 02:13 PM, Paul Berry wrote:<br>
> This reduces confusion since gl_shader::Type is sometimes<br>
> GL_SHADER_PROGRAM_MESA but is more frequently<br>
> GL_SHADER_{VERTEX,GEOMETRY,FRAGMENT}. It also has the advantage that<br>
> when switching on gl_shader::Stage, the compiler will alert if one of<br>
> the possible enum types is unhandled. Finally, many functions in<br>
> src/glsl (especially those dealing with linking) already use<br>
> gl_shader_stage to represent pipeline stages; storing a<br>
> gl_shader_stage in the gl_shader object avoids the need for a<br>
> conversion.<br>
<br>
</div>If you'll permit me a bit of grumbling...<br>
<br>
This patch does more than it says. The clear purpose of this patch is<br>
to add a new field. (Which is a non-trivial amount of work - moving<br>
definitions around, adding new code to set it in at least 4 places...)<br>
<br>
But then it goes and does a bunch of follow-on cleaning to /use/ the new<br>
field in a bunch of random places. This would've been great as a second<br>
patch...but alas.<br>
<br>
Not worth changing at this point, but for future reference, please...if<br>
you're going to rename something, rename that one thing. If you're<br>
going to add a field, add that one field. More patches are fine.<br></blockquote><div><br></div><div>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:<br>
<br>glsl: make _mesa_shader_stage_to_string() available to non-C++ code.<br>mesa: Move declaration of gl_shader_stage earlier in mtypes.h.<br>mesa: Store gl_shader_stage enum in gl_shader objects.<br>mesa: Use gl_shader::Stage instead of gl_shader::Type where possible. <br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
[snip]<br>
<div class="im"><br>
> diff --git a/src/mesa/program/prog_print.c b/src/mesa/program/prog_print.c<br>
> index fa120cc..9391e99 100644<br>
> --- a/src/mesa/program/prog_print.c<br>
> +++ b/src/mesa/program/prog_print.c<br>
> @@ -1005,16 +1005,21 @@ _mesa_print_parameter_list(const struct gl_program_parameter_list *list)<br>
> void<br>
> _mesa_write_shader_to_file(const struct gl_shader *shader)<br>
> {<br>
> - const char *type;<br>
> + const char *type = "????";<br>
<br>
</div>As far as I can tell this is a spurious change which is neither:<br>
- adding the new enum to the field (supposed purpose of patch)<br>
- changing things to use the new field directly (secondary change done<br>
in this patch)<br></blockquote><div><br></div><div>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.<br>
<br>I've made a note in the commit message of the behavioural change so that it doesn't come as a surprise to people.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Other than those minor grumbles, though, I really like what you're doing<br>
here!<br>
<br>
As is, the series is:<br>
Reviewed-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>><br></blockquote><div><br></div><div>Thanks for the review!<br></div></div></div></div>