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:
Reviewed-by: Carl Worth <cwo...@cworth.org>
-Carl
pgpACSuAGGWsf.pgp
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
