On Fri, 2004-01-23 at 07:44, Ben Reser wrote:
> On Wed, Jan 21, 2004 at 03:25:04PM -0000, bliss@tigris.org wrote:
> > I'd really like this issue fixed, but the current patch has several bugs in
> > diff_file.c. I have not checked the other files for similar errors. If you
> > compile on a platform with 32-bit apr_off_t, gcc even tells you about it:
> >
> > diff_file.c: In function `read_chunk':
> > diff_file.c:113: warning: passing arg 3 of `apr_file_seek' from incompatible
> > pointer type
> > diff_file.c: In function `svn_diff__file_output_unified_flush_hunk':
> > diff_file.c:709: warning: long int format, different type arg (arg 4)
> > diff_file.c:714: warning: long int format, different type arg (arg 4)
> > diff_file.c:719: warning: long int format, different type arg (arg 4)
> > diff_file.c:724: warning: long int format, different type arg (arg 4)
> >
> > All of these are serious errors. They are not hard too hard to fix, though.
>
> Okay this took more work to fix than I thought initially. The error on
> line 113 came from using apr_file_seek instead of svn_io_file_seek.
>
> I've converted it using svn_io_file_seek there but read_chunk (and a
> bunch of the libsvn_diff interface) didn't take a pool argument. This
> lead to some changes needed to the API for libsvn_diff.
>
> It was discussed on IRC this afternoon if we should remove the need for
> the pool param to svn_io_file_seek (which we could do) or should add a
> pool param to diff. The consensus of those who responded was to add
> pool to diff.
Well, no. If you need a pool in any of the functions you put in
your callback tables: svn_diff_fns_t or svn_diff_output_fns_t,
put it in the private baton you pass into the svn_diff_diff* or
svn_diff_output call.
> On the diff issue I changed more of the interfaces than probably
> necessary. If svn_diff_contains_conflicts and svn_diff_contains_diffs
Those are read only functions and will never need a pool.
> or the token_discard and token_discard_all (part of svn_diff_fns_t)
> needed to take pool parameters is questionable.
See above.
> I just did all of them
> now. If we don't like all of them taking pool parameters it's probably
> easier to remove some of the changes than it would be to go back and add
> them to the patch. So I did this in the interest of expediency of
> getting this issue closed.
>
> The new patch for the issue is Patch 289 on Issue #1705.
>
> You'll also want to apply the Patch 290 on Issue #1705 on top of that
> patch. I left them split up to make it easier to review them.
>
> This time I'm positive there are no warnings on 32-bit archs and it make
> check's on a 32-bit arch and 64-bit arch.
> Hopefully with this we can consider the issue done.
How on earth did we get ourselves into this mess? Isn't it easier
to 'fix Perl' as Joe Orton suggests on dev@apr, which seems the
biggest motivator for this change, and be done with it?
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Jan 23 19:55:36 2004