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

[Carl Worth]

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:

Reviewed-by: Carl Worth <cworth at cworth.org>

-Carl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130123/fb3e80ee/attachment.pgp>