apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rainer Jung <rainer.j...@kippdata.de>
Subject Re: [PATCH] Simplify and constify the new apr_hash_this_{key,key_len,val} functions
Date Fri, 29 Mar 2013 12:51:36 GMT
On 23.01.2013 16:15, Julian Foad wrote:
> Dear APR devs,
> 
> Following the relatively recent addition to APR trunk [1,2,3] of these functions:
> 
>   const char * apr_hash_this_key(apr_hash_index_t *);
>   apr_ssize_t  apr_hash_this_key_len(apr_hash_index_t *);
>   void *       apr_hash_this_val(apr_hash_index_t *);
> 
> I offer the attached patch as a simple and obvious improvement.

I considered this but I have the following comments:

- I don't like the code duplication between apr_hash_this() and the
apr_hash_this_*() functions (the "simplification" part). That way the
impl details of apr_hash_this() spread out to other functions. I'd
expect a compiler to be able to optimize the calls in a similar way.

- The constification produces warnings if we don't apply the
simplification as well. Adding the constness to apr_hash_this to get rid
of the warnings is probably not possible due to the versioning rules (at
least in APR 1.x).

Regards,

Rainer

> Log message:
> [[[
> Constify and simplify some hash indexing functions.
> 
> * include/apr_hash.h
>   (apr_hash_this_key, apr_hash_this_key_len, apr_hash_this_val): Constify
>     the hash index parameter.
> 
> * tables/apr_hash.c
>   (apr_hash_this_key, apr_hash_this_key_len, apr_hash_this_val): Simplify.
> ]]]
> 
> Patch (in case the attachment is hard to read):
> [[[
> Index: include/apr_hash.h
> ===================================================================
> --- include/apr_hash.h  (revision 1432927)
> +++ include/apr_hash.h  (working copy)
> @@ -171,21 +171,21 @@ APR_DECLARE(void) apr_hash_this(apr_hash
>   * @param hi The iteration state
>   * @return The pointer to the key
>   */
> -APR_DECLARE(const void*) apr_hash_this_key(apr_hash_index_t *hi);
> +APR_DECLARE(const void *) apr_hash_this_key(const apr_hash_index_t *hi);
> 
>  /**
>   * Get the current entry's key length from the iteration state.
>   * @param hi The iteration state
>   * @return The key length
>   */
> -APR_DECLARE(apr_ssize_t) apr_hash_this_key_len(apr_hash_index_t *hi);
> +APR_DECLARE(apr_ssize_t) apr_hash_this_key_len(const apr_hash_index_t *hi);
> 
>  /**
>   * Get the current entry's value from the iteration state.
>   * @param hi The iteration state
>   * @return The pointer to the value
>   */
> -APR_DECLARE(void*) apr_hash_this_val(apr_hash_index_t *hi);
> +APR_DECLARE(void *) apr_hash_this_val(const apr_hash_index_t *hi);
> 
>  /**
>   * Get the number of key/value pairs in the hash table.
> Index: tables/apr_hash.c
> ===================================================================
> --- tables/apr_hash.c   (revision 1432927)
> +++ tables/apr_hash.c   (working copy)
> @@ -162,28 +162,19 @@ APR_DECLARE(void) apr_hash_this(apr_hash
>      if (val)  *val  = (void *)hi->this->val;
>  }
> 
> -APR_DECLARE(const void *) apr_hash_this_key(apr_hash_index_t *hi)
> +APR_DECLARE(const void *) apr_hash_this_key(const apr_hash_index_t *hi)
>  {
> -    const void *key;
> -
> -    apr_hash_this(hi, &key, NULL, NULL);
> -    return key;
> +    return hi->this->key;
>  }
> 
> -APR_DECLARE(apr_ssize_t) apr_hash_this_key_len(apr_hash_index_t *hi)
> +APR_DECLARE(apr_ssize_t) apr_hash_this_key_len(const apr_hash_index_t *hi)
>  {
> -    apr_ssize_t klen;
> -
> -    apr_hash_this(hi, NULL, &klen, NULL);
> -    return klen;
> +    return hi->this->klen;
>  }
> 
> -APR_DECLARE(void *) apr_hash_this_val(apr_hash_index_t *hi)
> +APR_DECLARE(void *) apr_hash_this_val(const apr_hash_index_t *hi)
>  {
> -    void *val;
> -
> -    apr_hash_this(hi, NULL, NULL, &val);
> -    return val;
> +    return (void *)hi->this->val;
>  }
> 
>  /*
> ]]]
> 
> - Julian
> 
> 
> [1] tracked as: <https://issues.apache.org/bugzilla/show_bug.cgi?id=49065>
> 
> [2] committed as: <http://svn.apache.org/viewvc?view=revision&revision=931973>
> 
> [3] discussed at: <http://mail-archives.apache.org/mod_mbox/apr-dev/201003.mbox/%3C4B96A8EF.8030401%40rowe-clan.net%3E>
in the thread found at <http://mail-archives.apache.org/mod_mbox/apr-dev/201003.mbox/browser>;
search for "[PATCH] apr_hash_this_{key,klen,val}".

Mime
View raw message