[Mesa-dev] [PATCH 01/12] mesa: Refactor element type checking into its own function

[Ian Romanick]

On 08/22/2012 11:36 PM, Kenneth Graunke wrote:
> On 08/22/2012 07:26 PM, Ian Romanick wrote:
>> From: Ian Romanick <ian.d.romanick at intel.com>
>>
>> This consolidates the tests and makes the emitted error message
>> consistent.
>>
>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>> ---
>>   src/mesa/main/api_validate.c |   46 +++++++++++++++++++++---------------------
>>   1 files changed, 23 insertions(+), 23 deletions(-)
>>
>> diff --git a/src/mesa/main/api_validate.c b/src/mesa/main/api_validate.c
>> index eaf614b..e8c8333 100644
>> --- a/src/mesa/main/api_validate.c
>> +++ b/src/mesa/main/api_validate.c
>> @@ -262,6 +262,25 @@ _mesa_valid_prim_mode(struct gl_context *ctx, GLenum mode, const char *name)
>>      return GL_TRUE;
>>   }
>>
>> +/**
>> + * Verify that the element type is valid.
>> + *
>> + * Generates \c GL_INVALID_ENUM and returns \c false if it is not.
>> + */
>> +static bool
>> +_mesa_valid_element_type(struct gl_context *ctx, GLenum type, const char *name)
>> +{
>
> Could we call this _mesa_valid_draw_elements_type()?  When I first saw
> this, I was thinking more broadly..."elements of what?", "only unsigned
> elements are allowed?", and so on.
>
>> +   switch (type) {
>> +   case GL_UNSIGNED_BYTE:
>> +   case GL_UNSIGNED_SHORT:
>> +   case GL_UNSIGNED_INT:
>> +      return true;
>> +
>> +   default:
>> +      _mesa_error(ctx, GL_INVALID_ENUM, "%s(type = 0x%04x)", name, type);
>
> Could we also please do:
>
>        _mesa_error(ctx, GL_INVALID_ENUM, "%s(type = %s)", name,
>                    _mesa_lookup_enum_by_nr(type));
>
> May as well make it a bit friendlier, right?  These enums are
> unambiguous so the string will be correct.

Maybe.  This is a case when an app is passing in a value that could 
never be valid, so who knows what it passed.  0?  1234?  42?  When a 
value that could sometimes be valid is rejected, I like to print the 
string.  When a value that could never be valid is rejected (and may not 
even be an enum), the string may or may not be more friendly.

I don't have too strong of any opinion either way.

> Otherwise, I like the idea.
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>
>> +      return false;
>> +   }
>> +}
>>
>>   /**
>>    * Error checking for glDrawElements().  Includes parameter checking
>> @@ -286,13 +305,8 @@ _mesa_validate_DrawElements(struct gl_context *ctx,
>>         return GL_FALSE;
>>      }
>>
>> -   if (type != GL_UNSIGNED_INT &&
>> -       type != GL_UNSIGNED_BYTE &&
>> -       type != GL_UNSIGNED_SHORT)
>> -   {
>> -      _mesa_error(ctx, GL_INVALID_ENUM, "glDrawElements(type)" );
>> +   if (!_mesa_valid_element_type(ctx, type, "glDrawElements"))
>>         return GL_FALSE;
>> -   }
>>
>>      if (!check_valid_to_render(ctx, "glDrawElements"))
>>         return GL_FALSE;
>> @@ -348,13 +362,8 @@ _mesa_validate_MultiDrawElements(struct gl_context *ctx,
>>         return GL_FALSE;
>>      }
>>
>> -   if (type != GL_UNSIGNED_INT &&
>> -       type != GL_UNSIGNED_BYTE &&
>> -       type != GL_UNSIGNED_SHORT)
>> -   {
>> -      _mesa_error(ctx, GL_INVALID_ENUM, "glMultiDrawElements(type)" );
>> +   if (!_mesa_valid_element_type(ctx, type, "glMultiDrawElements"))
>>         return GL_FALSE;
>> -   }
>>
>>      if (!check_valid_to_render(ctx, "glMultiDrawElements"))
>>         return GL_FALSE;
>> @@ -419,12 +428,8 @@ _mesa_validate_DrawRangeElements(struct gl_context *ctx, GLenum mode,
>>         return GL_FALSE;
>>      }
>>
>> -   if (type != GL_UNSIGNED_INT &&
>> -       type != GL_UNSIGNED_BYTE &&
>> -       type != GL_UNSIGNED_SHORT) {
>> -      _mesa_error(ctx, GL_INVALID_ENUM, "glDrawRangeElements(type)" );
>> +   if (!_mesa_valid_element_type(ctx, type, "glDrawRangeElements"))
>>         return GL_FALSE;
>> -   }
>>
>>      if (!check_valid_to_render(ctx, "glDrawRangeElements"))
>>         return GL_FALSE;
>> @@ -548,13 +553,8 @@ _mesa_validate_DrawElementsInstanced(struct gl_context *ctx,
>>         return GL_FALSE;
>>      }
>>
>> -   if (type != GL_UNSIGNED_INT &&
>> -       type != GL_UNSIGNED_BYTE &&
>> -       type != GL_UNSIGNED_SHORT) {
>> -      _mesa_error(ctx, GL_INVALID_ENUM,
>> -                  "glDrawElementsInstanced(type=0x%x)", type);
>> +   if (!_mesa_valid_element_type(ctx, type, "glDrawElementsInstanced"))
>>         return GL_FALSE;
>> -   }
>>
>>      if (numInstances <= 0) {
>>         if (numInstances < 0)
>>