On 3 March 2015 at 14:17, Julian Foad <julianfoad_at_btopenworld.com> wrote:
>>>> URL: http://svn.apache.org/r1663450
>
>>>> Log:
>>>> Following up on r1658194, fix removing tokens from the tokens cache.
>>>> Just passing the token to svn_hash_sets() didn't work any more.
>
> Thinking about how to prevent a repeat of the same kind of error, defining svn_hash_sets and svn_hash_gets as functions with prototypes would result in at least a compiler warning (for typical configurations).
>
> One way of doing this is:
>
> Index: subversion/include/svn_hash.h
> ===================================================================
> --- subversion/include/svn_hash.h (revision 1663350)
> +++ subversion/include/svn_hash.h (working copy)
> @@ -246,2 +246,5 @@
> -#define svn_hash_gets(ht, key) \
> - apr_hash_get(ht, key, APR_HASH_KEY_STRING)
> +static APR_INLINE void *
> +svn_hash_gets(apr_hash_t *ht, const char *key)
> +{
> + return apr_hash_get(ht, key, APR_HASH_KEY_STRING);
> +}
> @@ -253,2 +256,5 @@
> -#define svn_hash_sets(ht, key, val) \
> - apr_hash_set(ht, key, APR_HASH_KEY_STRING, val)
> +static APR_INLINE void
> +svn_hash_sets(apr_hash_t *ht, const char *key, const void *val)
> +{
> + apr_hash_set(ht, key, APR_HASH_KEY_STRING, val);
> +}
>
> That particular way would likely trigger 'defined but not used' warnings on some
> source files, with compilers where APR_INLINE is not effective.
>
> Thoughts?
>
I think proposed patch could prevent this particular mistakes, but
this macro is in public header and I'm not sure that this change
doesn't break something.
Anyway I think it's better way to prevent such bugs is to stop
micro-optimizing Subversion. At least think twice before such commits.
I'd like to note that we've just fixed very similar bug with replacing
char * to svn_string_t month ago [1],[2]. And now we got similar
mistake.
[1] http://svn.apache.org/r1483570
[2] http://svn.haxx.se/dev/archive-2015-02/0107.shtml
--
Ivan Zhakov
Received on 2015-03-03 13:10:11 CET