On Wed, 2008-09-24 at 11:08 -0400, Paul Burba wrote:
> 2008/9/23 Julian Foad <julianfoad_at_btopenworld.com>:
> > Paul and others,
> >
> > I noticed that some of the code in libsvn_client/merge.c seems to be
> > implementing "search in a sorted array" and "insert an element into an
> > array" in its own way. I would like to see those bits of algorithm moved
> > out to somewhere generic so they don't clutter the mergeinfo-specific
> > code.
>
> Hi Julian,
>
> This looks great. Just a few minor comments below.
> I ran the tests over ra_neon and they passed as well.
Thanks. And I've now run them with svnserve.
> > svn_sorts.c isn't necessarily the right place for the array manipulation
> > functions, but searching and inserting do go hand-in-hand with sorting.
> > svn_sorts.h isn't necessarily the right header for them if defined as
> > private functions, but we already have a private "svn_sort__hash()"
> > there. We can move them to somewhere better if we like.
>
> svn_sorts.h seems as good a place as any.
Agreed. (If we want to move the private declarations to the "private"
subdirectory, we can do so as a separate change.)
> > +/* Return a deep copy of the merge-path structure OLD, allocated in POOL. */
> > +static svn_client__merge_path_t *
> > +svn_client__merge_path_dup(const svn_client__merge_path_t *old,
> > + apr_pool_t *pool)
> > +{
> > + svn_client__merge_path_t *new = apr_pmemdup(pool, old, sizeof(*old));
> >
> > - CHILDREN_WITH_MERGEINFO is a depth first sorted array filled with
> > - svn_client__merge_path_t *. Insert INSERT_ELEMENT into the
> > - CHILDREN_WITH_MERGEINFO array at index INSERT_INDEX. */
> > + new->path = apr_pstrdup(pool, old->path);
>
> What about the svn_client__merge_path_t members,
>
> apr_array_header_t *remaining_ranges
> svn_mergeinfo_t pre_merge_mergeinfo
> svn_mergeinfo_t implicit_mergeinfo
>
> shouldn't we make a deep copy of those as well? As the patch stands
> now all the callers of insert_child_to_merge (which is the only caller
> of svn_client__merge_path_dup) haven't populated these members yet so
> we are safe...for now.
Good point. It would be nasty to introduce a "dup" function that only
works for this particular user. Will fix.
> > +void
> > +svn_sort__array_insert(const void *new_element,
> > + apr_array_header_t *array,
> > + int insert_index)
> > +{
> > + int elements_to_move = array->nelts - insert_index; /* before growing */
> > + char *new_position; /* calculate after realloc */
>
> IIt might be advisable to check that 0 <= INSERT_INDEX <=
> ARRAY->NELTS. Something like:
>
> + int elements_to_move;
> + char *new_position; /* calculate after realloc */
> +
> + SVN_ERR_ASSERT(0 <= insert_index <= array->nelts)
> + elements_to_move = array->nelts - insert_index; /* before growing */
Sure. Will do.
> > + /* Allocate a new space at the end of the array. Note: this can
> > + reallocate the array at a different address. */
> > + apr_array_push(array);
> > +
> > + /* Move the elements after INSERT_INDEX along */
> > + new_position = (char *)array->elts + insert_index * array->elt_size;
> > + memmove(new_position + array->elt_size, new_position,
> > + array->elt_size * elements_to_move);
>
> If we are inserting to the end of an array or into an empty array we
> don't need to call memmove. This isn't really necessary, but
> marginally improves readability.
"If" statements in general tend to introduce opportunities for code
paths to diverge in buggy ways. I always try to make the edge cases be
handled by the standard code path if at all possible. However, a comment
that this is intentional might be useful. I'll add one.
> Other than those few concerns I am +1 on this.
Thanks. Will commit a cleaned up version.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-09-24 17:25:46 CEST