Re: [RFC] Simplify use of apr_hash_this()

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.

Also...

> 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.

> 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*

>...
> 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.

> Is this a good version that I can go ahead with now?

Not yet :-P

Cheers,
-g

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2368457
Received on 2009-07-06 19:54:00 CEST