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?
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.
> +
> +typedef struct svn_diff_hat_t svn_diff_hat_t;
> +
> +struct svn_diff_hat_t
> +{
> + svn_diff_hat_t *next;
> + apr_pool_t *pool;
> + svn_diff_lcs_t *links[1];
> +};
> +
In most (all?) of the existing code static function names do not begin
with 'svn_'.
> +static
> +apr_status_t
> +svn_diff__hat_create(svn_diff_hat_t **hat, apr_pool_t *pool)
> +{
> + svn_diff_hat_t *newhat;
> +
> + *hat = NULL;
> +
> + newhat = apr_pcalloc(pool, sizeof(svn_diff_hat_t) + 4095 * sizeof(svn_diff_lcs_t *));
A magic number! Why? How did you choose it?
> + newhat->pool = pool;
> +
> + *hat = newhat;
> +
> + return APR_SUCCESS;
> +}
> +
> +static
> +void
> +svn_diff__hat_set(svn_diff_hat_t *hat, apr_size_t idx, svn_diff_lcs_t *link)
> +{
> + apr_size_t size;
> + svn_diff_hat_t *newhat;
> +
> + size = 4096;
Another one, I assume it's related to the previous one...
> + while (idx > size
> + && hat->next != NULL)
> + {
> + idx -= size;
> + size <<= 1;
> + hat = hat->next;
> + }
> +
> + while (idx > size)
> + {
> + newhat = apr_pcalloc(hat->pool,
> + sizeof(svn_diff_hat_t)
> + + (size - 1) * sizeof(svn_diff_lcs_t *));
> + newhat->pool = hat->pool;
> +
> + hat->next = newhat;
> + hat = hat->next;
> +
> + idx -= size;
> + size <<= 1;
> + }
> +
> + hat->links[idx] = link;
> +}
> +
> +static
> +svn_diff_lcs_t *
> +svn_diff__hat_get(svn_diff_hat_t *hat, apr_size_t idx)
> +{
> + apr_size_t size;
> +
> + size = 4096;
Here it is again!
> + while (idx > size
> + && hat->next != NULL)
> + {
> + idx -= size;
> + size <<= 1;
> + hat = hat->next;
> + }
> +
> + if (hat == NULL)
> + {
> + return NULL;
> + }
> +
> + return hat->links[idx];
> +}
[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(...);
> +
> + diff = diff->next;
> + }
> +}
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?
Finally, I assume you are using some sort of test suite/harness, any
reason it has not been committed?
--
Philip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Apr 25 02:48:37 2002