[Mesa-dev] [PATCH 1/6] glsl: Eliminate ambiguity between function ins/outs and shader ins/outs

[Paul Berry]

On 23 January 2013 09:01, Carl Worth <cworth at cworth.org> wrote:

> Paul Berry <stereotype441 at 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 <cworth at cworth.org>
>
> -Carl
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130123/484c63cb/attachment.html>