You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Rall <dl...@collab.net> on 2007/09/10 19:56:16 UTC
Re: svn commit: r26507 - trunk/subversion/libsvn_client
We're not making a deep copy of merge_path_t's fields here (e.g. for
foo->path). Does that currently result in a problem (e.g. due to
differing pool lifetime)? (It certainly could later when others
modify the code.)
On Mon, 10 Sep 2007, kameshj@tigris.org wrote:
...
> Code Cleanup.
>
> * subversion/libsvn_client/merge.c
> (insert_child_to_merge): Instead of 'member by member copy',
> follow *ptr1=*ptr2 style of copy, so that things look simple.
...
> --- trunk/subversion/libsvn_client/merge.c (original)
> +++ trunk/subversion/libsvn_client/merge.c Mon Sep 10 04:57:25 2007
> @@ -3335,12 +3335,7 @@
> merge_path_t *);
> merge_path_t *curr_copy = apr_palloc(children_with_mergeinfo->pool,
> sizeof(*curr_copy));
> - curr_copy->path = curr->path;
> - curr_copy->missing_child = curr->missing_child;
> - curr_copy->switched = curr->switched;
> - curr_copy->has_noninheritable = curr->has_noninheritable;
> - curr_copy->absent = curr->absent;
> - curr_copy->propval = curr->propval;
> + *curr_copy = *curr;
> APR_ARRAY_PUSH(children_with_mergeinfo, merge_path_t *) = curr_copy;
>
> /* Move all elements from INSERT_INDEX to the end of the array forward
> @@ -3350,24 +3345,12 @@
> merge_path_t *prev;
> curr = APR_ARRAY_IDX(children_with_mergeinfo, j, merge_path_t *);
> if (j == insert_index)
> - {
> - curr->path = insert_element->path;
> - curr->missing_child = insert_element->missing_child;
> - curr->switched = insert_element->switched;
> - curr->has_noninheritable = insert_element->has_noninheritable;
> - curr->absent = insert_element->absent;
> - curr->propval = insert_element->propval;
> - }
> + *curr = *insert_element;
> else
> {
> prev = APR_ARRAY_IDX(children_with_mergeinfo, j - 1,
> merge_path_t *);
> - curr->path = prev->path;
> - curr->missing_child = prev->missing_child;
> - curr->switched = prev->switched;
> - curr->has_noninheritable = prev->has_noninheritable;
> - curr->absent = prev->absent;
> - curr->propval = prev->propval;
> + *curr = *prev;
> }
> }
> }
Re: svn commit: r26507 - trunk/subversion/libsvn_client
Posted by Kamesh Jayachandran <ka...@collab.net>.
> I wondered this too. From what I can tell, the pool lifetime is the
> same: we're copying an object from one place in an array to another, all
> within the array's pool.
>
> Regards,
> Malcolm
>
Yes, you are correct.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r26507 - trunk/subversion/libsvn_client
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Mon, Sep 10, 2007 at 12:56:16PM -0700, Daniel Rall wrote:
> We're not making a deep copy of merge_path_t's fields here (e.g. for
> foo->path). Does that currently result in a problem (e.g. due to
> differing pool lifetime)? (It certainly could later when others
> modify the code.)
>
I wondered this too. From what I can tell, the pool lifetime is the
same: we're copying an object from one place in an array to another, all
within the array's pool.
Regards,
Malcolm
Re: svn commit: r26507 - trunk/subversion/libsvn_client
Posted by Daniel Rall <dl...@collab.net>.
On Tue, 11 Sep 2007, Kamesh Jayachandran wrote:
...
> Yes we are not making a deep copy here. Till r26505 we had a
> inconsistent code of duplicating curr->path and not doing so for
> curr->propval. After careful examination I leaned towards shallow copy.
>
> Currently shallow copy does not cause any problem and eventually won't,
> unless someone keeps reference to pointers of *last* 'merge_path_t'
> *across* code.
> I would lean towards a 'Doc string stating the shallow copy behaviour'
> rather than deep copying things.
This sounds fine in private routines like this, so long as it is
clearly documented.
> Daniel Rall wrote:
> >We're not making a deep copy of merge_path_t's fields here (e.g. for
> >foo->path). Does that currently result in a problem (e.g. due to
> >differing pool lifetime)? (It certainly could later when others
> >modify the code.)
> >
> >On Mon, 10 Sep 2007, kameshj@tigris.org wrote:
> >...
> >
> >>Code Cleanup.
> >>
> >>* subversion/libsvn_client/merge.c
> >> (insert_child_to_merge): Instead of 'member by member copy',
> >> follow *ptr1=*ptr2 style of copy, so that things look simple.
> >>
> >...
> >
> >>--- trunk/subversion/libsvn_client/merge.c (original)
> >>+++ trunk/subversion/libsvn_client/merge.c Mon Sep 10 04:57:25 2007
> >>@@ -3335,12 +3335,7 @@
> >> merge_path_t *);
> >> merge_path_t *curr_copy = apr_palloc(children_with_mergeinfo->pool,
> >> sizeof(*curr_copy));
> >>- curr_copy->path = curr->path;
> >>- curr_copy->missing_child = curr->missing_child;
> >>- curr_copy->switched = curr->switched;
> >>- curr_copy->has_noninheritable = curr->has_noninheritable;
> >>- curr_copy->absent = curr->absent;
> >>- curr_copy->propval = curr->propval;
> >>+ *curr_copy = *curr;
> >> APR_ARRAY_PUSH(children_with_mergeinfo, merge_path_t *) =
> >> curr_copy;
> >>
> >> /* Move all elements from INSERT_INDEX to the end of the array
> >> forward
> >>@@ -3350,24 +3345,12 @@
> >> merge_path_t *prev;
> >> curr = APR_ARRAY_IDX(children_with_mergeinfo, j, merge_path_t
> >> *);
> >> if (j == insert_index)
> >>- {
> >>- curr->path = insert_element->path;
> >>- curr->missing_child = insert_element->missing_child;
> >>- curr->switched = insert_element->switched;
> >>- curr->has_noninheritable =
> >>insert_element->has_noninheritable;
> >>- curr->absent = insert_element->absent;
> >>- curr->propval = insert_element->propval;
> >>- }
> >>+ *curr = *insert_element;
> >> else
> >> {
> >> prev = APR_ARRAY_IDX(children_with_mergeinfo, j - 1,
> >> merge_path_t *);
> >>- curr->path = prev->path;
> >>- curr->missing_child = prev->missing_child;
> >>- curr->switched = prev->switched;
> >>- curr->has_noninheritable = prev->has_noninheritable;
> >>- curr->absent = prev->absent;
> >>- curr->propval = prev->propval;
> >>+ *curr = *prev;
> >> }
> >> }
> >> }
> >>
Re: svn commit: r26507 - trunk/subversion/libsvn_client
Posted by Kamesh Jayachandran <ka...@collab.net>.
Dan,
Yes we are not making a deep copy here. Till r26505 we had a
inconsistent code of duplicating curr->path and not doing so for
curr->propval. After careful examination I leaned towards shallow copy.
Currently shallow copy does not cause any problem and eventually won't,
unless someone keeps reference to pointers of *last* 'merge_path_t'
*across* code.
I would lean towards a 'Doc string stating the shallow copy behaviour'
rather than deep copying things.
Thanks.
With regards
Kamesh Jayachandran
Daniel Rall wrote:
> We're not making a deep copy of merge_path_t's fields here (e.g. for
> foo->path). Does that currently result in a problem (e.g. due to
> differing pool lifetime)? (It certainly could later when others
> modify the code.)
>
> On Mon, 10 Sep 2007, kameshj@tigris.org wrote:
> ...
>
>> Code Cleanup.
>>
>> * subversion/libsvn_client/merge.c
>> (insert_child_to_merge): Instead of 'member by member copy',
>> follow *ptr1=*ptr2 style of copy, so that things look simple.
>>
> ...
>
>> --- trunk/subversion/libsvn_client/merge.c (original)
>> +++ trunk/subversion/libsvn_client/merge.c Mon Sep 10 04:57:25 2007
>> @@ -3335,12 +3335,7 @@
>> merge_path_t *);
>> merge_path_t *curr_copy = apr_palloc(children_with_mergeinfo->pool,
>> sizeof(*curr_copy));
>> - curr_copy->path = curr->path;
>> - curr_copy->missing_child = curr->missing_child;
>> - curr_copy->switched = curr->switched;
>> - curr_copy->has_noninheritable = curr->has_noninheritable;
>> - curr_copy->absent = curr->absent;
>> - curr_copy->propval = curr->propval;
>> + *curr_copy = *curr;
>> APR_ARRAY_PUSH(children_with_mergeinfo, merge_path_t *) = curr_copy;
>>
>> /* Move all elements from INSERT_INDEX to the end of the array forward
>> @@ -3350,24 +3345,12 @@
>> merge_path_t *prev;
>> curr = APR_ARRAY_IDX(children_with_mergeinfo, j, merge_path_t *);
>> if (j == insert_index)
>> - {
>> - curr->path = insert_element->path;
>> - curr->missing_child = insert_element->missing_child;
>> - curr->switched = insert_element->switched;
>> - curr->has_noninheritable = insert_element->has_noninheritable;
>> - curr->absent = insert_element->absent;
>> - curr->propval = insert_element->propval;
>> - }
>> + *curr = *insert_element;
>> else
>> {
>> prev = APR_ARRAY_IDX(children_with_mergeinfo, j - 1,
>> merge_path_t *);
>> - curr->path = prev->path;
>> - curr->missing_child = prev->missing_child;
>> - curr->switched = prev->switched;
>> - curr->has_noninheritable = prev->has_noninheritable;
>> - curr->absent = prev->absent;
>> - curr->propval = prev->propval;
>> + *curr = *prev;
>> }
>> }
>> }
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org