Re: [RFC] Simplify use of apr_hash_this()

On Mon, 2009-07-06 at 19:53 +0200, Greg Stein wrote:
> On Mon, Jul 6, 2009 at 19:25, Julian Foad<julianfoad_at_btopenworld.com> wrote:
> >...
> > OK, I've attached a patch, <apr_hash_this-def-2.patch>.
>
> The concept is good, though I'd suggest changing the HI parameter to a
> const, and un-const'ing it the implementation. When these move into
> APR, they should be made const.

OK. (I wondered about this and decided that there was a de-facto
assumption in APR that all references to 'hi' should be non-const even
when logically constant. But I will be glad to have the API
const-correct. It will give us "casting away const" warnings in the
implementations but I think it's OK to have a small fixed number of them
for reasons like this.)

> > I chose the names apr_hash_index_key(), apr_hash_index_klen(),
> > apr_hash_index_val(). They are in APR namespace because, like our
> > APR_ARRAY_IDX and APR_ARRAY_PUSH, they are APR-level functions that are
> > candidates for going into APR. (APR_ARRAY_IDX and APR_ARRAY_PUSH made it
> > into APR 1.3.)
>
> Those are preprocessor macros, and are easy to *avoid* defining when
> the correct APR comes along.
>
> That is not the case for functions.
>
> Consider what happens when they become defined in (say) APR 1.5. Now
> the symbol is visible in *both* libapr, and libsvn. Not good.
>
> Not to mention duplicate declaration fiascos.

OK. The prefix "svn_hash_" is already used to refer to a text dump
format, so how about

  svn_apr_hash_index_key()
  svn_apr_hash_index_klen()
  svn_apr_hash_index_val()

Those names are getting a bit long but maybe not too long. I think
they're OK.

> > The definitions are in svn_types.h, because they are needed throughout
> > the code base and we have had other APR helper APIs there.
>
> Yes.
>
> > The implementations are in subversion/libsvn_subr/iter.c because,
> > although their use is an alternative rather than a complement to
> > svn_iter_*(), they are closely related.
>
> *shrug*

(Not a great reason, I know. Just a good enough place for starters. It's
easy to move them within libsvn_subr if we think of a better place.)

> > This now works, and you can see how much simpler it makes the usage in
> > the attached <apr_hash_this-use-2.patch>. In this patch I have
> > simplified the usage in the first several C files in
> > subversion/libsvn_client/ and all the C files in subversion/svn*/.
>
> Yup. Definitely cleaner.
>
> I'd even suggest that in some cases, the intermediate variable can go
> away entirely. For cases like:
>
> const char *value = apr_hash_index_val(hi);
>
> some_function(value);
>
> Could be:
>
> some_function(apr_hash_index_val(hi));
>
> Since the cast will be implied by some_function's prototype.

Yes, that will be a valid option at the point of use.

> > Is this a good version that I can go ahead with now?
>
> Not yet :-P

Thanks for the quick review.

This version?

- Julian

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2368510