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.
-Hyrum
Received on 2011-01-27 22:14:30 CET