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

[Brian Paul]

On 09/13/2012 02:44 PM, Kristian Høgsberg wrote:
> 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.

Ah, OK.  I think i was confusing probing the hash table vs. the values 
table.  I still think a comment would be helpful. :)

-Brian