On 3 March 2015 at 14:17, Julian Foad <julianf...@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
