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