You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@wandisco.com> on 2010/04/20 10:49:40 UTC
Avoiding svn_iter_apr_hash/array() [was: svn commit: r935579]
On Mon, 2010-04-19, Greg Stein wrote:
> On Mon, Apr 19, 2010 at 10:03, <ju...@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
Re: Avoiding svn_iter_apr_hash/array() [was: svn commit: r935579]
Posted by Greg Stein <gs...@gmail.com>.
Hehe... reminds me of my short fight against SVN_ERR hiding a return
statement, way back when...
On Apr 20, 2010 11:46 AM, "Hyrum K. Wright" <hy...@mail.utexas.edu>
wrote:
On Tue, Apr 20, 2010 at 10:34 AM, Julian Foad <ju...@wandisco.com>
wrote:
>
> Daniel Shahaf wr...
While an interesting exercise in extending the C language, I've got serious
reservations about introducing these types of macros. They don't improve
code clarity, and make it more difficult for a novice to the code to
understand what is going on. Please think long and hard before adding these
obfuscating "features" to our code base.
-Hyrum
Re: Avoiding svn_iter_apr_hash/array() [was: svn commit: r935579]
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Tue, Apr 20, 2010 at 10:34 AM, Julian Foad <ju...@wandisco.com>wrote:
> Daniel Shahaf wrote:
> > Julian Foad wrote on Tue, 20 Apr 2010 at 11:49 +0100:
> > > 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:
> >
> > A code block inside a macro call??
>
> Well, I tried it and it works and I kind of like it... but really I
> tried it for fun and I'm not sure if we should be seriously considering
> anything like this.
>
> > Perhaps we write a macro in the
> > style of SVN_DBG --- one that is "pseudo-variadic"...
> >
> > For example (not tested):
> >
> > #define SVN_ITER_ARRAY2(element_type, element_name, array, iterpool,
> pool, func_name, (arg1, arg2)) \
> > SVN_ITER_ARRAY(element_type, element_name, array, iterpool, pool,
> func_name(arg1, arg2))
>
> I think you mean something like (with a backslash after every line):
>
> #define SVN_ITER_ARRAY2(element_type, element_name, array,
> iterpool, pool,
> func_name, arg_list)
> SVN_ITER_ARRAY(element_type, element_name, array,
> iterpool, pool,
> {
> svn_error_t *__err = func_name arg_list;
> if (__err->apr_err == SVN_ERR_ITER_BREAK)
> {
> svn_error_clear(err);
> break;
> }
> SVN_ERR(__err);
> }
>
> And I think the advantages of your SVN_ITER_ARRAY2 are:
>
> * not taking a code block as an argument, thus avoiding the surprise
> (and possible confusion of syntax-highlighting editors etc.);
>
> and its disadvantages are:
>
> * the body of the loop has to be expressed as a single function call,
> which adds a considerable overhead for simple operations.
>
> - Julian
>
>
> > where SVN_ITER_ARRAY() is the macro you defined:
> >
> > > #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); \
> > > }
> > >
> >
> > :-)
> >
> > Daniel
> >
> > > 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
>
While an interesting exercise in extending the C language, I've got serious
reservations about introducing these types of macros. They don't improve
code clarity, and make it more difficult for a novice to the code to
understand what is going on. Please think long and hard before adding these
obfuscating "features" to our code base.
-Hyrum
Re: Avoiding svn_iter_apr_hash/array() [was: svn commit: r935579]
Posted by Julian Foad <ju...@wandisco.com>.
Daniel Shahaf wrote:
> Julian Foad wrote on Tue, 20 Apr 2010 at 11:49 +0100:
> > 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:
>
> A code block inside a macro call??
Well, I tried it and it works and I kind of like it... but really I
tried it for fun and I'm not sure if we should be seriously considering
anything like this.
> Perhaps we write a macro in the
> style of SVN_DBG --- one that is "pseudo-variadic"...
>
> For example (not tested):
>
> #define SVN_ITER_ARRAY2(element_type, element_name, array, iterpool, pool, func_name, (arg1, arg2)) \
> SVN_ITER_ARRAY(element_type, element_name, array, iterpool, pool, func_name(arg1, arg2))
I think you mean something like (with a backslash after every line):
#define SVN_ITER_ARRAY2(element_type, element_name, array,
iterpool, pool,
func_name, arg_list)
SVN_ITER_ARRAY(element_type, element_name, array,
iterpool, pool,
{
svn_error_t *__err = func_name arg_list;
if (__err->apr_err == SVN_ERR_ITER_BREAK)
{
svn_error_clear(err);
break;
}
SVN_ERR(__err);
}
And I think the advantages of your SVN_ITER_ARRAY2 are:
* not taking a code block as an argument, thus avoiding the surprise
(and possible confusion of syntax-highlighting editors etc.);
and its disadvantages are:
* the body of the loop has to be expressed as a single function call,
which adds a considerable overhead for simple operations.
- Julian
> where SVN_ITER_ARRAY() is the macro you defined:
>
> > #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); \
> > }
> >
>
> :-)
>
> Daniel
>
> > 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
> >
> >
> >
Re: Avoiding svn_iter_apr_hash/array() [was: svn commit: r935579]
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Tue, 20 Apr 2010 at 11:49 +0100:
> 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:
>
A code block inside a macro call?? Perhaps we write a macro in the
style of SVN_DBG --- one that is "pseudo-variadic"...
For example (not tested):
#define SVN_ITER_ARRAY2(element_type, element_name, array, iterpool, pool, func_name, (arg1, arg2)) \
SVN_ITER_ARRAY(element_type, element_name, array, iterpool, pool, func_name(arg1, arg2))
where SVN_ITER_ARRAY() is the macro you defined:
> #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); \
> }
>
:-)
Daniel
> 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
>
>
>