[Mesa-dev] [PATCH 2/3] glsl: Clarify and document type_compare()

[Paul Berry]

On 26 July 2011 16:08, Chad Versace <chad at chad-versace.us> wrote:
> The parameters were named 'a' and 'b', but there was no documentation
> indicating which was being converted to which. So rename 'a' -> 'to' and
> 'b' -> 'from'.
>
> Also add a Doxygen comment.
>
> Signed-off-by: Chad Versace <chad at chad-versace.us>
> ---
>  src/glsl/ir_function.cpp |   27 +++++++++++++++++----------
>  1 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/src/glsl/ir_function.cpp b/src/glsl/ir_function.cpp
> index 6795988..e6ba359 100644
> --- a/src/glsl/ir_function.cpp
> +++ b/src/glsl/ir_function.cpp
> @@ -24,29 +24,36 @@
>  #include "glsl_types.h"
>  #include "ir.h"
>
> +/**
> + * \brief Check if an implicit type conversion is possible.
> + *
> + * Called by type_compare().
> + *
> + * \return If conversion is possible, 1. Else, -1.

Actually, if I'm not mistaken, it's:
0 if the types match exactly.
1 if the types don't match, but a conversion is possible.
-1 if the types don't match, and a conversion is impossible.

> + */
>  int
> -type_compare(const glsl_type *a, const glsl_type *b)
> +type_compare(const glsl_type *to, const glsl_type *from)
>  {
>    /* If the types are the same, they trivially match.
>     */
> -   if (a == b)
> +   if (to == from)
>       return 0;
>
> -   switch (a->base_type) {
> +   switch (to->base_type) {
>    case GLSL_TYPE_UINT:
>    case GLSL_TYPE_INT:
>    case GLSL_TYPE_BOOL:
>       /* There is no implicit conversion to or from integer types or bool.
>        */
> -      if ((a->is_integer() != b->is_integer())
> -         || (a->is_boolean() != b->is_boolean()))
> +      if ((to->is_integer() != from->is_integer())
> +         || (to->is_boolean() != from->is_boolean()))
>         return -1;
>
>       /* FALLTHROUGH */
>
>    case GLSL_TYPE_FLOAT:
> -      if ((a->vector_elements != b->vector_elements)
> -         || (a->matrix_columns != b->matrix_columns))
> +      if ((to->vector_elements != from->vector_elements)
> +         || (to->matrix_columns != from->matrix_columns))
>         return -1;
>
>       return 1;
> @@ -58,8 +65,8 @@ type_compare(const glsl_type *a, const glsl_type *b)
>       return -1;
>
>    case GLSL_TYPE_ARRAY:
> -      if ((b->base_type != GLSL_TYPE_ARRAY)
> -         || (a->length != b->length))
> +      if ((from->base_type != GLSL_TYPE_ARRAY)
> +         || (to->length != from->length))
>         return -1;
>
>       /* From GLSL 1.50 spec, page 27 (page 33 of the PDF):
> @@ -68,7 +75,7 @@ type_compare(const glsl_type *a, const glsl_type *b)
>        * If the comparison of the array element types detects that a conversion
>        * would be required, the array types do not match.
>        */
> -      return (type_compare(a->fields.array, b->fields.array) == 0) ? 0 : -1;
> +      return (type_compare(to->fields.array, from->fields.array) == 0) ? 0 : -1;
>
>    case GLSL_TYPE_VOID:
>    case GLSL_TYPE_ERROR:
> --
> 1.7.6
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>

I realize this is a bit of yak shaving, but I'm not thrilled about
type_compare's three possible return values.  I'd prefer to move
responsibility for checking whether the types match exactly into the
caller.  Then type_compare would only need two return values, so it
could return a boolean, and we could name the function in such a way
that the meaning of the boolean was obvious.  For example:

bool is_conversion_possible(const glsl_type *to, const glsl_type *from);

Any chance we could do this bit of clean-up while we're in the neighborhood?