[Mesa-dev] [PATCH 3/3] mesa: Fix string_to_uint_map->iterate to return same numbers from put()

[Ian Romanick]

On 12/04/2014 02:00 PM, Carl Worth wrote:
> There is an internal implementation detail that the hash table
> underlying the struct string_to_uint_map stores each value internally
> as (value+1). The user needn't be very concerned with this (other than
> knowing that a value of UINT_MAX cannot be stored) since put() adds 1
> and get() subtracts 1.
> 
> However, the recently added iterate() method was written to directly
> leak the internal, off-by-one values, which could be quite
> confusing. Fix this with a little wrapper around the user's callback
> function to perform the subtraction.
> 
> And now that we have a wrapper in place, we give a better signature to
> the callback function being passed to iterate(), so that this callback
> function can actually expect a char* and an unsigned argument, (rather
> than a couple of void* ). So the iterate() method should be much more
> convenient to use now.

Should this just get squashed with the previous commit?  Either way,
this patch and the previous patch are

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

One tiny nit below.

> ---
>  src/mesa/program/hash_table.h | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mesa/program/hash_table.h b/src/mesa/program/hash_table.h
> index ece43a1..c7089b2 100644
> --- a/src/mesa/program/hash_table.h
> +++ b/src/mesa/program/hash_table.h
> @@ -198,6 +198,11 @@ string_to_uint_map_dtor(struct string_to_uint_map *);
>  #ifdef __cplusplus
>  }
>  
> +struct string_map_iterate_wrapper_closure {
> +   void (*callback)(const char *key, unsigned value, void *closure);
> +   void *closure;
> +};
> +
>  /**
>   * Map from a string (name) to an unsigned integer value
>   *
> @@ -231,9 +236,19 @@ public:
>     /**
>      * Runs a passed callback for the hash
>      */
> -   void iterate(void (*func)(const void *, void *, void *), void *closure)
> +   void iterate(void (*func)(const char *, unsigned, void *), void *closure)
>     {
> -      hash_table_call_foreach(this->ht, func, closure);
> +      struct string_map_iterate_wrapper_closure *wrapper;
> +
> +      wrapper = (struct string_map_iterate_wrapper_closure *)
> +         malloc(sizeof(struct string_map_iterate_wrapper_closure));
> +      if (wrapper == NULL)
> +         return;
> +
> +      wrapper->callback = func;
> +      wrapper->closure = closure;
> +
> +      hash_table_call_foreach(this->ht, subtract_one_wrapper, wrapper);
>     }
>  
>     /**
> @@ -289,6 +304,16 @@ private:
>        free((char *)key);
>     }
>  
> +   static void subtract_one_wrapper(const void *key, void *data, void *closure)
> +   {
> +      struct string_map_iterate_wrapper_closure *wrapper =
> +         (struct string_map_iterate_wrapper_closure *) closure;
> +      unsigned value = (intptr_t) data;
> +
> +      value -= 1;
> +
> +      wrapper->callback((const char *) key, value, wrapper->closure);
> +   }

Blank line here.

>     struct hash_table *ht;
>  };
>  
>