<div dir="ltr">On 23 January 2013 09:01, Carl Worth <span dir="ltr"><<a href="mailto:cworth@cworth.org" target="_blank">cworth@cworth.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> writes:<br>
> This patch replaces the three ir_variable_mode enums:<br>
</div>...<br>
> with the following five:<br>
<br>
In my review of this patch, I'm not familiar with the code enough to<br>
know a priori which things should become "var" and which should be come<br>
"function". I did look for obvious cues (such as a "formal_param" should<br>
be associated with "function" of course). Otherwise, I've just reviewed<br>
for contextual consistency. And using those criteria, the renaming seems<br>
sound.<br>
<br>
One thing I do like to take special care is that in a big-rename patch<br>
like this, we should be careful to not let unrelated code changes slip<br>
in. The only thing I saw along these lines looks quite minor:<br>
<div class="im"><br>
>     switch (var->mode) {<br>
>     case ir_var_auto:<br>
> -   case ir_var_in:<br>
> -   case ir_var_const_in:<br>
> +   case ir_var_shader_in:<br>
>     case ir_var_uniform:<br>
</div>...<br>
<div class="im">>     default:<br>
> +      /* The only variables that are added using this function should be<br>
> +       * uniforms, shader inputs, and shader outputs, constants (which use<br>
> +       * ir_var_auto), and system values.<br>
> +       */<br>
>        assert(0);<br>
<br>
</div>This removal of "case ir_var_const_in" (and justification in the<br>
comment) seems unrelated to the rest of the patch and is not described<br>
in the commit message. I can't really comment on how obviously safe this<br>
code change may or may not be, but perhaps this piece should be placed<br>
in a separate commit; that's your call. Either way:<br></blockquote><div><br></div><div>Ok, I can go along with that.  Thanks, Carl.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
Reviewed-by: Carl Worth <<a href="mailto:cworth@cworth.org">cworth@cworth.org</a>><br>
<span class="HOEnZb"><font color="#888888"><br>
-Carl<br>
</font></span></blockquote></div><br></div></div>