Hyrum K. Wright wrote on Sat, 13 Sep 2008 at 13:21 -0500:
> Philip Martin wrote:
> > hwright_at_tigris.org writes:
> >> Directly return some errors, removing superfluous invocations of SVN_ERR().
> >> This only handles libsvn_client; the other libraries could be similarly
> >> audited.
> >>
> >> I tried not to be too agressive when doing this. There's a balance between
> >> maintainability and efficiency: there are probably other places where we can
> >> directly return, but tracking those down is tricky, and can introduce subtle
> >> bugs for future maintainers.
> >
> > Looks like pointless code churn to me; every active branch is now
> > facing a massive merge. What is the gain?
>
> Aside from the slight performance gain,
We have bigger performance problems than using SVN_ERR() immediately
before a return. :)
> >> @@ -1939,26 +1922,23 @@ setup_copy(svn_commit_info_t **commit_in
> >> if ((! srcs_are_urls) && (! dst_is_url))
> >> {
> >> *commit_info_p = NULL;
> >> - SVN_ERR(wc_to_wc_copy(copy_pairs, is_move, make_parents,
> >> - ctx, pool));
> >> + return wc_to_wc_copy(copy_pairs, is_move, make_parents, ctx, pool);
> >> }
> >> else if ((! srcs_are_urls) && (dst_is_url))
> >> {
> >> - SVN_ERR(wc_to_repos_copy(commit_info_p, copy_pairs, make_parents,
> >> - revprop_table, ctx, pool));
> >> + return wc_to_repos_copy(commit_info_p, copy_pairs, make_parents,
> >> + revprop_table, ctx, pool);
> >> }
> >> else if ((srcs_are_urls) && (! dst_is_url))
> >> {
> >> *commit_info_p = NULL;
> >> - SVN_ERR(repos_to_wc_copy(copy_pairs, make_parents, ctx, pool));
> >> + return repos_to_wc_copy(copy_pairs, make_parents, ctx, pool);
> >> }
> >> else
> >> {
> >> - SVN_ERR(repos_to_repos_copy(commit_info_p, copy_pairs, make_parents,
> >> - revprop_table, ctx, is_move, pool));
> >> + return repos_to_repos_copy(commit_info_p, copy_pairs, make_parents,
> >> + revprop_table, ctx, is_move, pool);
> >> }
> >> -
> >> - return SVN_NO_ERROR;
> >> }
> >
> > It might be an improvement in short functions,
+1
> > but in cases like the
> > code above it's not clear that your new code is better--if pushed I
> > probably prefer the original code.
>
> In this case, and others like it, I disagree. By directly returning, we help
> show the organization of the code: setup_copy() performs some common work and
> then dispatches the remainder of the processing to the respective helper functions.
>
When I see "return svn_foo()" in a function that doesn't return
a boolean or an int, I just get confused -- what is being returned?
The code we write doesn't need to reflect the fact that we implement
exception handling through return type.
> But, that's just my feeling, if there is strong sentiment to the contrary, we
> can revert it.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-09-13 20:54:15 CEST