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
Those names are getting a bit long but maybe not too long. I think
> > The definitions are in svn_types.h, because they are needed throughout
> > the code base and we have had other APR helper APIs there.
> > 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.
(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);
> Could be:
> 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.
Received on 2009-07-06 21:58:04 CEST