You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Julian Foad <ju...@btopenworld.com> on 2013/01/23 16:15:04 UTC
[PATCH] Simplify and constify the new apr_hash_this_{key,key_len,val} functions
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.
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}".
Re: [PATCH] Simplify and constify the new apr_hash_this_{key,key_len,val} functions
Posted by Igor Galić <i....@brainsware.org>.
On Wednesday, January 23, 2013 03:15:04 PM 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 just noticed that no one's applied or replied to Julian's patch yet.
it seems like a good idea and the implementation looks sane enough.
Not only that, it also has documentation!
-- i
> 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.8
> 030401%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}".
Re: [PATCH] Simplify and constify the new apr_hash_this_{key,key_len,val}
functions
Posted by Rainer Jung <ra...@kippdata.de>.
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}".