You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ju...@apache.org on 2015/03/03 19:09:32 UTC
svn commit: r1663760 - in /subversion/trunk/subversion: include/svn_hash.h
libsvn_subr/hash.c
Author: julianfoad
Date: Tue Mar 3 18:09:32 2015
New Revision: 1663760
URL: http://svn.apache.org/r1663760
Log:
Redefine the svn_hash_gets and svn_hash_sets macros in a way that allows for
parameter type checking, at least in debug builds.
* subversion/include/svn_hash.h
(svn_hash__gets): New function.
(svn_hash_gets): In debug mode, call svn_hash__gets; in release mode, call
apr_hash_get directly. (The prototype of apr_hash_get checks the type of
the 'ht' parameter but not of the 'key'.)
(svn_hash_sets): Rewrite so as to assign the 'key' parameter to a 'const
char *' variable and so get type checking.
* subversion/libsvn_subr/hash.c
(svn_hash__gets): New function.
Modified:
subversion/trunk/subversion/include/svn_hash.h
subversion/trunk/subversion/libsvn_subr/hash.c
Modified: subversion/trunk/subversion/include/svn_hash.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_hash.h?rev=1663760&r1=1663759&r2=1663760&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_hash.h (original)
+++ subversion/trunk/subversion/include/svn_hash.h Tue Mar 3 18:09:32 2015
@@ -239,19 +239,37 @@ svn_hash_from_cstring_keys(apr_hash_t **
const apr_array_header_t *keys,
apr_pool_t *pool);
+#ifdef SVN_DEBUG
+/* In debug builds, the svn_hash_gets macro forwards the parameters
+ * through this function in order to have parameter type checking,
+ * particularly for the key. The svn_hash_sets macro gets parameter
+ * type checking in a different way, by using an in-line assignment.
+ */
+void *
+svn_hash__gets(apr_hash_t *ht, const char *key);
+#endif
+
/** Shortcut for apr_hash_get() with a const char * key.
*
* @since New in 1.8.
*/
+#ifdef SVN_DEBUG
+#define svn_hash_gets(ht, key) \
+ svn_hash__gets(ht, key)
+#else
#define svn_hash_gets(ht, key) \
apr_hash_get(ht, key, APR_HASH_KEY_STRING)
+#endif
/** Shortcut for apr_hash_set() with a const char * key.
*
* @since New in 1.8.
*/
-#define svn_hash_sets(ht, key, val) \
- apr_hash_set(ht, key, APR_HASH_KEY_STRING, val)
+#define svn_hash_sets(ht, key, val) \
+ do { \
+ const char *svn_hash__key = (key); \
+ apr_hash_set(ht, svn_hash__key, APR_HASH_KEY_STRING, val); \
+ } while (0)
/** @} */
Modified: subversion/trunk/subversion/libsvn_subr/hash.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/hash.c?rev=1663760&r1=1663759&r2=1663760&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/hash.c (original)
+++ subversion/trunk/subversion/libsvn_subr/hash.c Tue Mar 3 18:09:32 2015
@@ -560,6 +560,13 @@ svn_hash_from_cstring_keys(apr_hash_t **
}
+void *
+svn_hash__gets(apr_hash_t *ht, const char *key)
+{
+ return apr_hash_get(ht, key, APR_HASH_KEY_STRING);
+}
+
+
/*** Specialized getter APIs ***/
Re: svn commit: r1663760 - svn_hash_sets() type checking
Posted by Julian Foad <ju...@btopenworld.com>.
Daniel Shahaf wrote:
>> URL: http://svn.apache.org/r1663760
[...]
>> -#define svn_hash_sets(ht, key, val) \
>> - apr_hash_set(ht, key, APR_HASH_KEY_STRING, val)
>> +#define svn_hash_sets(ht, key, val) \
>> + do { [...] } while (0)
>
> Strictly speaking, that's a backwards-incompatible change. Since the
> new definition is no longer an rvalue, API consumers' code would break
> if they used svn_hash_sets() as part of a statement. Example:
>
> /* This works with svn 1.8. */
> #include <subversion-1/svn_hash.h>
> #define my_foo() (svn_hash_sets(h, s), 42)
>
> I think we have to either errata this, or restore svn_hash_gets()'
> previous definition and name the do-while version svn_hash_gets2().
>
> Makes sense?
Yes... but a nicer solution would be to define it the same way as I defined *get* (forward through a private function), wouldn't it? That would restore a symmetry that Brane pointed out was surprisingly missing.
r1663896.
- Julian
Re: svn commit: r1663760 - svn_hash_sets() type checking
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
julianfoad@apache.org wrote on Tue, Mar 03, 2015 at 18:09:32 -0000:
> Author: julianfoad
> Date: Tue Mar 3 18:09:32 2015
> New Revision: 1663760
>
> URL: http://svn.apache.org/r1663760
> Log:
> Redefine the svn_hash_gets and svn_hash_sets macros in a way that allows for
> parameter type checking, at least in debug builds.
>
> /** Shortcut for apr_hash_set() with a const char * key.
> *
> * @since New in 1.8.
> */
> -#define svn_hash_sets(ht, key, val) \
> - apr_hash_set(ht, key, APR_HASH_KEY_STRING, val)
> +#define svn_hash_sets(ht, key, val) \
> + do { \
> + const char *svn_hash__key = (key); \
> + apr_hash_set(ht, svn_hash__key, APR_HASH_KEY_STRING, val); \
> + } while (0)
Strictly speaking, that's a backwards-incompatible change. Since the
new definition is no longer an rvalue, API consumers' code would break
if they used svn_hash_sets() as part of a statement. Example:
/* This works with svn 1.8. */
#include <subversion-1/svn_hash.h>
#define my_foo() (svn_hash_sets(h, s), 42)
I think we have to either errata this, or restore svn_hash_gets()'
previous definition and name the do-while version svn_hash_gets2().
Makes sense?
Cheers,
Daniel
Re: svn commit: r1663760 - svn_hash_sets() type checking
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
julianfoad@apache.org wrote on Tue, Mar 03, 2015 at 18:09:32 -0000:
> Author: julianfoad
> Date: Tue Mar 3 18:09:32 2015
> New Revision: 1663760
>
> URL: http://svn.apache.org/r1663760
> Log:
> Redefine the svn_hash_gets and svn_hash_sets macros in a way that allows for
> parameter type checking, at least in debug builds.
>
> /** Shortcut for apr_hash_set() with a const char * key.
> *
> * @since New in 1.8.
> */
> -#define svn_hash_sets(ht, key, val) \
> - apr_hash_set(ht, key, APR_HASH_KEY_STRING, val)
> +#define svn_hash_sets(ht, key, val) \
> + do { \
> + const char *svn_hash__key = (key); \
> + apr_hash_set(ht, svn_hash__key, APR_HASH_KEY_STRING, val); \
> + } while (0)
Strictly speaking, that's a backwards-incompatible change. Since the
new definition is no longer an rvalue, API consumers' code would break
if they used svn_hash_sets() as part of a statement. Example:
/* This works with svn 1.8. */
#include <subversion-1/svn_hash.h>
#define my_foo() (svn_hash_sets(h, s), 42)
I think we have to either errata this, or restore svn_hash_gets()'
previous definition and name the do-while version svn_hash_gets2().
Makes sense?
Cheers,
Daniel