[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Avoiding svn_iter_apr_hash/array() [was: svn commit: r935579]

From: Julian Foad <julian.foad_at_wandisco.com>
Date: Tue, 20 Apr 2010 11:49:40 +0100

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

This is an archived mail posted to the Subversion Dev mailing list.