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

[Kristian Høgsberg]

On Tue, Sep 11, 2012 at 1:13 PM, Brian Paul <brianp at vmware.com> wrote:
> 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.

No, nothing that subtle.  The hash function will give an initial index
into 'table', which just has an index into  'values'.  The values
entry is the description of how to find the value.  If that matches
the enum, we were trying to look up we're done.  Otherwise we add a
prime step to the index into 'table' and look up a new index into
'values' there.  We keep going like this until the index we look up in
'table' is 0, which is not a valid entry into 'values'.  There's no
tricky module arithmetic going on there, we just reached an entry in
'table' that no enum hashed to, which means that the enum we're trying
to look up is not in the table.

Kristian