[Mesa-dev] [PATCH v2 07/12] mesa: glGet: simplify the 'enum not found' condition

[Brian Paul]

On 09/11/2012 10:36 AM, Kristian Høgsberg wrote:
> On Tue, Sep 11, 2012 at 11:47 AM, Brian Paul<brianp at vmware.com>  wrote:
>> On 09/10/2012 12:41 AM, Imre Deak wrote:
>>>
>>> When traversing the hash table looking up an enum that is invalid we
>>> eventually reach the first element in the descriptor array. By looking
>>> at the type of that element, which is always TYPE_API_MASK, we know that
>>> we can stop the search and return error. Since this element is always
>>> the first it's enough to check for its index being 0 without looking at
>>> its type.
>>>
>>> Later in this patchset, when we generate the hash tables during build
>>> time, this will allow us to remove the TYPE_API_MASK and related flags
>>> completly.
>>>
>>> Signed-off-by: Imre Deak<imre.deak at intel.com>
>>> ---
>>>    src/mesa/main/get.c |    8 +++++---
>>>    1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/mesa/main/get.c b/src/mesa/main/get.c
>>> index 63fe296..48c6911 100644
>>> --- a/src/mesa/main/get.c
>>> +++ b/src/mesa/main/get.c
>>> @@ -2012,16 +2012,18 @@ find_value(const char *func, GLenum pname, void
>>> **p, union value *v)
>>>       mask = Elements(table(api)) - 1;
>>>       hash = (pname * prime_factor);
>>>       while (1) {
>>> -      d =&values[table(api)[hash&   mask]];
>>> +      int idx = table(api)[hash&   mask];
>>>
>>>
>>>          /* If the enum isn't valid, the hash walk ends with index 0,
>>> -       * which is the API mask entry at the beginning of values[]. */
>>> -      if (unlikely(d->type == TYPE_API_MASK)) {
>>> +       * pointing to the first entry of values[] which doesn't hold
>>> +       * any valid enum. */
>>> +      if (unlikely(!idx)) {
>>
>>
>> Minor nit, but I think "idx != 0" would be nicer here.
>>
>>
>>>             _mesa_error(ctx, GL_INVALID_ENUM, "%s(pname=%s)", func,
>>>                   _mesa_lookup_enum_by_nr(pname));
>>>             return&error_value;
>>>          }
>>>
>>> +      d =&values[idx];
>>>
>>>          if (likely(d->pname == pname))
>>>             break;
>>>
>>
>> To be honest, I'm not quite sure I understand how this code works and how we
>> wind up at entry[0] for an invalid enum.
>
> We use a hash table to map enum values to the index into the
> corresponding value_desc table.  The first entry in value_desc table
> is not a valid enum description, so we use index 0 as a terminator for
> the hash chains.  So if we end up looking at value_desc[0], it means
> that we reached the end of a hash chain and didn't file the value we
> were looking for.

And by always stepping by the prime step we'll eventually wind up at 
index 0, right?  I think that's the subtle part that could use a 
comment.

-Brian