Last reply before I really sleepzzz...
> From: Philip Martin [mailto:pm@localhost]On Behalf Of Philip Martin
> Sent: 25 April 2002 02:47
> striker@tigris.org writes:
>
>> --- trunk/subversion/libsvn_diff/diff.c (original)
>> +++ trunk/subversion/libsvn_diff/diff.c Tue Apr 23 15:02:56 2002
>
> [snip]
>
> > +/*
> > + * Support structure to implement 'large arrays'.
> > + */
>
> This is some sort of optimisation isn't it? Is there any profiling
> that shows a) that it is needed, or b) that it works?
Like I said in my post to the list; we need to investigate if we
can solve this with basic apr types. This was my implementation
to get something to work quickly.
> In the absence of profiling I would guess that for small to medium
> files on today's machines the runtime will be dominated by file I/O,
> but that all it is, a guess.
It all depends on what you diff. It is certainly not limited to files.
[...]
> In most (all?) of the existing code static function names do not begin
> with 'svn_'.
Oh fun... /me must have looked at the wrong file or made a wrong
assumpting in mind.
[...]
>> + newhat = apr_pcalloc(pool, sizeof(svn_diff_hat_t) + 4095 * sizeof(svn_diff_lcs_t *));
>
> A magic number! Why? How did you choose it?
I drew a magic circle on the floor, lit the candles, danced until it started to
rain and then decided on 4096 (-1 for the one already in the struct). This
is one of the things that would disappear entirely if we can use an apr type
for this. /me wonders if apr_array would work.
[...]
> > + size = 4096;
>
> Another one, I assume it's related to the previous one...
Yes.
> [snip]
>
> > +void
> > +svn_diff_output(svn_diff_t *diff,
> > + void *output_baton,
> > + svn_diff_output_fns_t *vtable)
> > +{
> > + while (diff != NULL)
> > + {
> > + switch (diff->type)
> > + {
> > + case svn_diff_type_common:
> > + if (vtable->output_common != NULL)
> > + {
> > + vtable->output_common(output_baton,
> > + diff->baseline_start,
> > + diff->baseline_length,
> > + diff->workingcopy_start,
> > + diff->workingcopy_length,
> > + diff->repository_start,
> > + diff->repository_length);
> > + }
> > + break;
> > +
> > + case svn_diff_type_diff_common:
> > + if (vtable->output_diff_common != NULL)
> > + {
> > + vtable->output_diff_common(output_baton,
> > + diff->baseline_start,
> > + diff->baseline_length,
> > + diff->workingcopy_start,
> > + diff->workingcopy_length,
> > + diff->repository_start,
> > + diff->repository_length);
> > + }
> > + break;
> > +
> > + case svn_diff_type_diff_workingcopy:
> > + if (vtable->output_diff_workingcopy != NULL)
> > + {
> > + vtable->output_diff_workingcopy(output_baton,
> > + diff->baseline_start,
> > + diff->baseline_length,
> > + diff->workingcopy_start,
> > + diff->workingcopy_length,
> > + diff->repository_start,
> > + diff->repository_length);
> > + }
> > + break;
> > +
> > + case svn_diff_type_diff_repository:
> > + if (vtable->output_diff_repository != NULL)
> > + {
> > + vtable->output_diff_repository(output_baton,
> > + diff->baseline_start,
> > + diff->baseline_length,
> > + diff->workingcopy_start,
> > + diff->workingcopy_length,
> > + diff->repository_start,
> > + diff->repository_length);
> > + }
> > + break;
> > +
> > + case svn_diff_type_conflict:
> > + if (vtable->output_conflict != NULL)
> > + {
> > + vtable->output_conflict(output_baton,
> > + diff->baseline_start,
> > + diff->baseline_length,
> > + diff->workingcopy_start,
> > + diff->workingcopy_length,
> > + diff->repository_start,
> > + diff->repository_length);
> > + }
> > + break;
> > + }
>
> More neatly written as:
>
> void (*output_function)(void*,apr_off_t,apr_off_t,apr_off_t,
> apr_off_t,apr_off_t,apr_off_t);
> /* or some_typedef_t output_function; */
>
> switch (...)
> {
> case xxx:
> output_function = vtable->yyy;
> break;
> }
>
> if (output_function)
> output_function(...);
Yup, nicer indeed. I started out with funcs that had different args.
Could do this now.
[...]
> How are you going to deal with files that don't have a newline at the
> end? To produce output like
>
> 5c5
> < E
> \ No newline at end of file
> ---
> > E
>
> or
>
> @@ -2,4 +2,4 @@
> B
> C
> D
> -E
> \ No newline at end of file
> +E
>
> I think one of the output functions will need to know that it is
> responsible for the last bit. How will this be communicated?
I always wondered why we needed that \No newline at end of file crap.
If there is no newline on the end of file, don't output a newline.
What's wrong with that?
> Finally, I assume you are using some sort of test suite/harness, any
> reason it has not been committed?
Yah, it's too simple/incomplete. We need to write a ton of tests for
this. Also, my local hacked up test doesn't fit the testsuite yet.
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Apr 25 03:15:08 2002