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}".