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

Re: Status of the branch diff-optimizations-bytes branch

From: Hyrum K Wright <hyrum_at_hyrumwright.org>
Date: Thu, 27 Jan 2011 22:31:09 -0600

On Thu, Jan 27, 2011 at 3:13 PM, Hyrum K Wright <hyrum_at_hyrumwright.org> wrote:
> On Thu, Jan 27, 2011 at 3:08 PM, Johan Corveleyn <jcorvel_at_gmail.com> wrote:
>> On Thu, Jan 27, 2011 at 4:52 PM, Hyrum K Wright <hyrum_at_hyrumwright.org> wrote:
>>> Johan,
>>> I'd appreciate review on the attached patch.  It is an attempt to rev
>>> the svn_diff_fns_t struct and related functions.  You'll notice that I
>>> commented out the use of datasources_open in both diff_file.c and
>>> diff_memory.c, in an attempt to exercise the deprecated function
>>> wrappers.
>>>
>>> It currently segfaults horribly, but I'm not completely sure why.  I
>>> suspect something is amiss in the implementation of the compat wrapper
>>> for datasources_open in deprecated.c.  If you'd review this patch, I'd
>>> be much obliged.
>>
>> I don't completely grok the rev'ing and wrapping stuff just yet, but
>> giving it a try ...
>>
>> One thing looked suspicious to me:
>>
>>> Index: subversion/libsvn_diff/deprecated.c
>>> ===================================================================
>>> --- subversion/libsvn_diff/deprecated.c       (revision 1064135)
>>> +++ subversion/libsvn_diff/deprecated.c       (working copy)
>>> @@ -41,7 +41,61 @@
>>>
>>>
>>>  /*** Code. ***/
>>> +struct fns_wrapper_baton
>>> +{
>>> +  /* We put the old baton in front of this one, so that we can still use
>>> +     this baton in place of the old.  This prevents us from having to
>>> +     implement simple wrappers around each member of diff_fns_t. */
>>> +  void *old_baton;
>>> +  const svn_diff_fns_t *vtable;
>>> +};
>>>
>>> +static svn_error_t *
>>> +datasources_open(void *baton, apr_off_t *prefix_lines,
>>> +                 svn_diff_datasource_e datasources[],
>>> +                 apr_size_t datasource_len)
>>> +{
>>> +  struct fns_wrapper_baton *fwb = baton;
>>> +  int i;
>>> +
>>> +  /* Just iterate over the datasources, using the old singular version. */
>>> +  for (i = 0; i < datasource_len; i++)
>>> +    {
>>> +      SVN_ERR(fwb->vtable->datasource_open(baton, datasources[i]));
>>
>> Not sure, but shouldn't that be 'baton->old_baton' instead of 'baton'?
>
> Technically, it shouldn't matter, since *baton and *old_baton are the
> same.  In fact, I'm exploiting that fact to avoid having to wrap *all*
> the callbacks.
>
> (But I could be wrong here, and Interesting Things could be happening.)
>
>>> +    }
>>> +
>>> +  /* Don't claim any prefix matches. */
>>> +  *prefix_lines = 0;
>>> +
>>> +  return SVN_NO_ERROR;
>>> +}
>>
>> The rest looks ok, at least as far as I understand the rev'ing stuff ...
>
> The bit I'm shaky about is how to emulate the behavior of
> datasources_open in terms of data_source open.  *Something* is not
> quite right since it continues to segfault.

For completeness: I committed an improved version of the patch in r1064347.

-Hyrum
Received on 2011-01-28 05:31:46 CET

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