You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2011/09/12 19:35:53 UTC
Re: svn commit: r1169837 -
/subversion/trunk/subversion/libsvn_delta/compat.c
hwright@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().
> (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 *
>
>
Re: svn commit: r1169837 -
/subversion/trunk/subversion/libsvn_delta/compat.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Hyrum K Wright wrote on Mon, Sep 12, 2011 at 16:11:04 -0500:
> On Mon, Sep 12, 2011 at 2:06 PM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > 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).
>
> As you point out, the removal of the additional branch depends upon
> the optimizer. I hardly think that means I'm "just wrong." Partially
> wrong, maybe, but it certainly isn't as final as you make it sound. :)
>
I fail to see your point here. Did you mean to say that my
characterization of your claim as wrong was itself wrong because your
claim is true when the code is compiled without optimizations?
> In any case, I was making a similar change elsewhere, and decided to
> remove some extra cruft. You may not agree with that change, and
> that's fine. But right now, this is all very new code, and
> (unfortunately) I'm the only person actively hacking it, so I'm kinda
> inclined to do what makes my life easier, so long as it doesn't hurt
> anything else.
>
It doesn't hurt anything else, but your log message described the
change as an "improvement", whereas I think it is a no op, so I pointed
that out.
Re: svn commit: r1169837 - /subversion/trunk/subversion/libsvn_delta/compat.c
Posted by Hyrum K Wright <hy...@wandisco.com>.
On Mon, Sep 12, 2011 at 2:06 PM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> 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).
As you point out, the removal of the additional branch depends upon
the optimizer. I hardly think that means I'm "just wrong." Partially
wrong, maybe, but it certainly isn't as final as you make it sound. :)
In any case, I was making a similar change elsewhere, and decided to
remove some extra cruft. You may not agree with that change, and
that's fine. But right now, this is all very new code, and
(unfortunately) I'm the only person actively hacking it, so I'm kinda
inclined to do what makes my life easier, so long as it doesn't hurt
anything else.
> 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.
And I see it as making the code more readable, which *is* a gain. But
that's just a personal preference upon which we can agree to disagree.
-Hyrum
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Re: svn commit: r1169837 -
/subversion/trunk/subversion/libsvn_delta/compat.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Mon, Sep 12, 2011 at 21:46:41 +0200:
> On Mon, Sep 12, 2011 at 10:06:26PM +0300, Daniel Shahaf wrote:
> > 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).
>
> [citation needed]
http://article.gmane.org/gmane.comp.version-control.subversion.devel/104238
Re: svn commit: r1169837 -
/subversion/trunk/subversion/libsvn_delta/compat.c
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Sep 12, 2011 at 10:06:26PM +0300, Daniel Shahaf wrote:
> 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).
[citation needed]
Re: svn commit: r1169837 -
/subversion/trunk/subversion/libsvn_delta/compat.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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....@daniel.shahaf.name> wrote:
> > hwright@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/
Re: svn commit: r1169837 -
/subversion/trunk/subversion/libsvn_delta/compat.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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....@daniel.shahaf.name> wrote:
> > hwright@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/
Re: svn commit: r1169837 - /subversion/trunk/subversion/libsvn_delta/compat.c
Posted by Hyrum K Wright <hy...@wandisco.com>.
On Mon, Sep 12, 2011 at 12:35 PM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> hwright@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/
Re: svn commit: r1169837 - /subversion/trunk/subversion/libsvn_delta/compat.c
Posted by Hyrum K Wright <hy...@wandisco.com>.
On Mon, Sep 12, 2011 at 12:35 PM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> hwright@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/