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

Re: svn commit: r1169837 - /subversion/trunk/subversion/libsvn_delta/compat.c

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Mon, 12 Sep 2011 22:06:26 +0300

Look. You're just wrong. SVN_ERR() always implies svn_error_trace()
regardless of SVN_ERR__TRACING, and there is no additional branch here
in an optimized build (and last time you were introducing those changes
someone demonstrated this via disassembly excerpts).

So, if you want to reduce on branches, then merge ^/subversion/branches/performance.
But as I see it such changes make the code less readable for no gain.

Hyrum K Wright wrote on Mon, Sep 12, 2011 at 13:05:58 -0500:
> On Mon, Sep 12, 2011 at 12:35 PM, Daniel Shahaf <d.s_at_daniel.shahaf.name> wrote:
> > hwright_at_apache.org wrote on Mon, Sep 12, 2011 at 17:27:29 -0000:
> >> Author: hwright
> >> Date: Mon Sep 12 17:27:28 2011
> >> New Revision: 1169837
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1169837&view=rev
> >> Log:
> >> A couple of minor improvements to the Ev2->delta editor shim.
> >>
> >> * subversion/libsvn_delta/compat.c
> >>   (complete_cb): Use svn_error_trace().
> >
> > Unnecessary change.  SVN_ERR() implies svn_error_trace().
>
> Only in the case of SVN_ERR_TRACING being defined.
>
> In any case, SVN_ERR() introduces an additional branch, which in this
> case is superfluous.
>
> >>   (abort_cb): Pass through to the delta editor.
> >>
> >> Modified:
> >>     subversion/trunk/subversion/libsvn_delta/compat.c
> >>
> >> Modified: subversion/trunk/subversion/libsvn_delta/compat.c
> >> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_delta/compat.c?rev=1169837&r1=1169836&r2=1169837&view=diff
> >> ==============================================================================
> >> --- subversion/trunk/subversion/libsvn_delta/compat.c (original)
> >> +++ subversion/trunk/subversion/libsvn_delta/compat.c Mon Sep 12 17:27:28 2011
> >> @@ -581,10 +581,8 @@ complete_cb(void *baton,
> >>              apr_pool_t *scratch_pool)
> >>  {
> >>    struct editor_baton *eb = baton;
> >> -
> >> -  SVN_ERR(eb->deditor->close_edit(eb->dedit_baton, scratch_pool));
> >> -
> >> -  return SVN_NO_ERROR;
> >> +  return svn_error_trace(eb->deditor->close_edit(eb->dedit_baton,
> >> +                                                 scratch_pool));
> >>  }
> >>
> >>  /* This implements svn_editor_cb_abort_t */
> >> @@ -592,7 +590,9 @@ static svn_error_t *
> >>  abort_cb(void *baton,
> >>           apr_pool_t *scratch_pool)
> >>  {
> >> -  return SVN_NO_ERROR;
> >> +  struct editor_baton *eb = baton;
> >> +  return svn_error_trace(eb->deditor->abort_edit(eb->dedit_baton,
> >> +                                                 scratch_pool));
> >>  }
> >>
> >>  svn_error_t *
> >>
> >>
> >
>
>
>
> --
>
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com/
Received on 2011-09-12 21:07:32 CEST

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

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.