On Mon, 2010-04-19, Greg Stein wrote:
> On Mon, Apr 19, 2010 at 10:03, <julianfoad_at_apache.org> wrote:
> > * subversion/libsvn_client/commit.c
> > (post_process_commit_item): Eliminate a misleadingly named local variable.
>
> The best way to clarify what is happening is to remove the use of
> svn_iter_apr_array().
>
> Consider: that "utility" function requires you to set up a callback
> function. That callback is going to consume more lines of code in the
> signature, and in the baton declaration, than a simple "for (i = 0; i
> < ary->nelts; ++i)" line. One line. Or maybe +3 more to
> create/clear/destroy an iterpool.
>
> Also, locating the core of the loop where you're actually trying to
> accomplish the work will result in more clarity.
>
> I said this the other day: svn_iter_* are bastard functions and should
> be eliminated from our code. They only serve to reduce code clarity.
I didn't hear you say that the other day, but I've just taken a look at
the iter functions and their usage, and I agree.
* One reason for them was to make it easier to correctly use an
"iterpool" - but we already have a well established, clear and easy way
to do that.
* One benefit that I assume was intended was to simplify the iteration
code at the point of use - but, due to the assignment of parameters into
a baton, that turns out to be not a simplification but a trade of one
complexity for another.
* An incidental benefit gained during the initial usage (in r866607),
quoted in the original log message, was factoring out common
functionality - but that was not a property of using the iter functions
and should have been done separately.
* Since those iteration functions were introduced, the typical hash
iteration point-of-use code has been simplified by the introduction of
svn__apr_hash_index_key/klen/val().
So that's barely any reason left for keeping them, as far as I can see.
I see only 7 uses of these svn_iter_apr_XXX() functions in our code base
(4 of one, 3 of the other).
I don't personally intend to change the current usages back to a regular
in-line iteration as a separate task, but I'd be happy to see it happen.
I might well do it on a case-by-case basis if I happen to be working on
any of the functions that use it.
The idea was good in principle - I'd love to be able to approach the
simplicity of iteration in Python etc. - but in practice the current
functions haven't achieved this simplicity.
Hmm... I wonder if we could do something with a macro that takes the
whole block of code to be executed as an argument, like:
#define SVN_ITER_ARRAY(element_type, element_name, array, \
iterpool_name, /* as subpool of */ pool, \
code_block) \
{ apr_pool_t *iterpool_name = ... \
for (...) \
{ \
element_type element_name = ...; \
apr_pool_clear(iterpool_name); \
code_block \
} \
apr_pool_destroy(iterpool_name); \
}
Usage:
SVN_ITER_ARRAY(svn_node_t *, node, nodes_array,
iterpool, existing_pool,
{
if (node->kind == svn_node_file)
SVN_ERR(bar(node, blah, iterpool));
else
break; /* breaks the iteration */
} ) /* macro ends here */
Hmm...
- Julian
Received on 2010-04-20 12:50:19 CEST