Re: [RFC] Simplify use of apr_hash_this()

On Wed, 2009-07-01 at 11:17 -0400, Greg Hudson wrote:
> On Wed, 2009-07-01 at 00:45 +0100, Julian Foad wrote:
> > In a minority of instances, we are using explicit casts:
>
> I observe that in every instance you cited, one of the arguments passed
> is NULL.

In those instances I cited and also throughout all our uses of
apr_hash_this(), in most cases both key and value are wanted, but the
key-length argument is NULL.

> This leads me to believe that the interfaces we really want are:
>
> const void *apr_hash_key(hi)
> void *apr_hash_val(hi)
>
> which nicely sidestep the aliasing issue by returning the value instead
> of using a pointer argument. Even in the (apparently rare) cases where
> we want the key and the value, it may be more elegant to make two
> function calls than to use a single call with temporaries, or a
> multi-line macro.

Even though we usually want both key and value, that could well be a
nice solution. For one thing, it enables us to initialize the variables
at their declaration. Even better than the hypothetical...

  for (hi = apr_hash_first(pool, entries); hi; hi = apr_hash_next(hi))
    {
      const char *item;
      svn_wc_entry_t *entry;

      apr_hash_this(hi, &item, NULL, &entry); /* WISHFUL THINKING */

      ...
    }

we could then write

  for (hi = apr_hash_first(pool, entries); hi; hi = apr_hash_next(hi))
    {
      const char *item = apr_hash_key(hi);
      svn_wc_entry_t *entry = apr_hash_val(hi);

      ...
    }

> Of course, those APIs do not exist in APR (right now), so we'd have to
> define them ourselves.

Just to point out for anyone reading this who may not be familiar with
the history: That's perfectly fine, and we have a precedent of defining
useful APR extensions that are sometimes later incoprporated into APR
itself.

- Julian

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2367091
Received on 2009-07-01 18:00:37 CEST