You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by kf...@collab.net on 2005/10/10 21:22:12 UTC
Re: [PATCH] dup function for svn_info_t
Chris, this seems not to have made it into Subversion. Do you not
need it anymore, or do you still want it but just were waiting for
someone to commit it?
-Karl
"Peter N. Lundblad" <pe...@famlundblad.se> writes:
> On Mon, 13 Jun 2005, Chris Foote wrote:
>
> > Branko Čibej wrote:
> > > Chris Foote wrote:
> > >
> > > >[LOG]
> > > >
> > > >Add a function to duplicate the svn_info_t structure.
> > > >
> > > >* subversion/include/svn_client.h
> > > > (svn_info_t): Add note to update svn_info_dup when extending the struct.
> > > > (svn_info_dup): New function to duplicate an svn_info_t.
> > > >
> > > >* subversion/libsvn_client/info.c
> > > > (svn_info_dup): Ditto.
> > > >
> > > >
> > > Why add a public function that's not used anywhere?
> > >
> > So that clients (i.e. one I'm writing) can create safe copies of the struct
> > that are allocated in a different pool from the one that was used when
> > calling svn_client_info.
> >
>
> I agree. The struct might be extended, so to be safe, you need such a
> function.
> > > >+svn_info_t *
> > > >+svn_info_dup (const svn_info_t *info, apr_pool_t *pool)
> > > >+{
> > > >+ svn_info_t *dupinfo = apr_pcalloc (pool, sizeof(*dupinfo));
> > > >+
> > > >+ /* Perform a trivial copy ... */
> > > >+ *dupinfo = *info;
> > > >
> > > >
> > > Since you're copying the struct anyway, there's no sense in using
> > > apr_pcalloc.
> > True, although this is what svn_wc_entry_dup uses.
> >
> I don't mind changing that too, even if this is really cycle-counting if
> anything...
>
> Regards,
> //Peter
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
--
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] dup function for svn_info_t
Posted by Chris Foote <cf...@v21.me.uk>.
Yep, still need it, just using a local copy until it's committed.
Thanks
Chris
----- Original Message -----
From: <kf...@collab.net>
To: "Peter N. Lundblad" <pe...@famlundblad.se>
Cc: "Chris Foote" <cf...@v21.me.uk>; "Branko Čibej" <br...@xbc.nu>;
<de...@subversion.tigris.org>
Sent: Monday, October 10, 2005 10:22 PM
Subject: Re: [PATCH] dup function for svn_info_t
Chris, this seems not to have made it into Subversion. Do you not
need it anymore, or do you still want it but just were waiting for
someone to commit it?
-Karl
"Peter N. Lundblad" <pe...@famlundblad.se> writes:
> On Mon, 13 Jun 2005, Chris Foote wrote:
>
> > Branko Čibej wrote:
> > > Chris Foote wrote:
> > >
> > > >[LOG]
> > > >
> > > >Add a function to duplicate the svn_info_t structure.
> > > >
> > > >* subversion/include/svn_client.h
> > > > (svn_info_t): Add note to update svn_info_dup when extending the
struct.
> > > > (svn_info_dup): New function to duplicate an svn_info_t.
> > > >
> > > >* subversion/libsvn_client/info.c
> > > > (svn_info_dup): Ditto.
> > > >
> > > >
> > > Why add a public function that's not used anywhere?
> > >
> > So that clients (i.e. one I'm writing) can create safe copies of the struct
> > that are allocated in a different pool from the one that was used when
> > calling svn_client_info.
> >
>
> I agree. The struct might be extended, so to be safe, you need such a
> function.
> > > >+svn_info_t *
> > > >+svn_info_dup (const svn_info_t *info, apr_pool_t *pool)
> > > >+{
> > > >+ svn_info_t *dupinfo = apr_pcalloc (pool, sizeof(*dupinfo));
> > > >+
> > > >+ /* Perform a trivial copy ... */
> > > >+ *dupinfo = *info;
> > > >
> > > >
> > > Since you're copying the struct anyway, there's no sense in using
> > > apr_pcalloc.
> > True, although this is what svn_wc_entry_dup uses.
> >
> I don't mind changing that too, even if this is really cycle-counting if
> anything...
>
> Regards,
> //Peter
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
--
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] dup function for svn_info_t
Posted by David James <ja...@gmail.com>.
On 10/14/05, David Anderson <da...@calixo.net> wrote:
> David James wrote:
>
> >>Chris, I've committed your patch in r16398 and proposed it for
> >>backport to the 1.3.x branch, because we need this function for the
> >>Ruby bindings. Your code is very easy to read, follows all of our
> >>project conventions, and of course works perfectly. Great work!
> >>
> >>
> >(I committed your patch in r16708, not r16398. Oops!)
> >
>
> Thanks David! I'll get reviewing later today, so that we can put the
> Ruby crashes to rest.
Great! This API function will be needed later to fix the Ruby crashes
(I'm working on a comprehensive patch to fix a large number of
pool-related scoping issues in the Ruby bindings, which I'm hoping to
finish this weekend).
Cheers,
David
--
David James -- http://www.cs.toronto.edu/~james
Re: [PATCH] dup function for svn_info_t
Posted by David Anderson <da...@calixo.net>.
David James wrote:
>>Chris, I've committed your patch in r16398 and proposed it for
>>backport to the 1.3.x branch, because we need this function for the
>>Ruby bindings. Your code is very easy to read, follows all of our
>>project conventions, and of course works perfectly. Great work!
>>
>>
>(I committed your patch in r16708, not r16398. Oops!)
>
Thanks David! I'll get reviewing later today, so that we can put the
Ruby crashes to rest.
- Dave.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] dup function for svn_info_t
Posted by Chris Foote <cf...@v21.me.uk>.
Thanks,
Chris
----- Original Message -----
From: "David James" <ja...@gmail.com>
To: <kf...@collab.net>; <de...@subversion.tigris.org>
Cc: "Peter N. Lundblad" <pe...@famlundblad.se>; "Chris Foote"
<cf...@v21.me.uk>; "Branko Čibej" <br...@xbc.nu>
Sent: Friday, October 14, 2005 1:36 PM
Subject: Re: [PATCH] dup function for svn_info_t
> On 10/14/05, David James <ja...@gmail.com> wrote:
> > On 10/10/05, David James <ja...@gmail.com> wrote:
> > > On 10 Oct 2005 16:22:12 -0500, kfogel@collab.net <kf...@collab.net>
wrote:
> > > > Chris, this seems not to have made it into Subversion. Do you not
> > > > need it anymore, or do you still want it but just were waiting for
> > > > someone to commit it?
> > > >
> > > > -Karl
> > > This patch looks perfect to me. In the Python bindings, we don't wrap
> > > svn_client_info yet, but when we do, we'll need the svn_info_dup
> > > function.
> > >
> > > If you are also happy with the patch, please commit it.
> > Chris, I've committed your patch in r16398 and proposed it for
> > backport to the 1.3.x branch, because we need this function for the
> > Ruby bindings. Your code is very easy to read, follows all of our
> > project conventions, and of course works perfectly. Great work!
> (I committed your patch in r16708, not r16398. Oops!)
>
> Cheers,
>
> David
>
> --
> David James -- http://www.cs.toronto.edu/~james
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] dup function for svn_info_t
Posted by David James <ja...@gmail.com>.
On 10/14/05, David James <ja...@gmail.com> wrote:
> On 10/10/05, David James <ja...@gmail.com> wrote:
> > On 10 Oct 2005 16:22:12 -0500, kfogel@collab.net <kf...@collab.net> wrote:
> > > Chris, this seems not to have made it into Subversion. Do you not
> > > need it anymore, or do you still want it but just were waiting for
> > > someone to commit it?
> > >
> > > -Karl
> > This patch looks perfect to me. In the Python bindings, we don't wrap
> > svn_client_info yet, but when we do, we'll need the svn_info_dup
> > function.
> >
> > If you are also happy with the patch, please commit it.
> Chris, I've committed your patch in r16398 and proposed it for
> backport to the 1.3.x branch, because we need this function for the
> Ruby bindings. Your code is very easy to read, follows all of our
> project conventions, and of course works perfectly. Great work!
(I committed your patch in r16708, not r16398. Oops!)
Cheers,
David
--
David James -- http://www.cs.toronto.edu/~james
Re: [PATCH] dup function for svn_info_t
Posted by David James <ja...@gmail.com>.
On 10/10/05, David James <ja...@gmail.com> wrote:
> On 10 Oct 2005 16:22:12 -0500, kfogel@collab.net <kf...@collab.net> wrote:
> > Chris, this seems not to have made it into Subversion. Do you not
> > need it anymore, or do you still want it but just were waiting for
> > someone to commit it?
> >
> > -Karl
> This patch looks perfect to me. In the Python bindings, we don't wrap
> svn_client_info yet, but when we do, we'll need the svn_info_dup
> function.
>
> If you are also happy with the patch, please commit it.
Chris, I've committed your patch in r16398 and proposed it for
backport to the 1.3.x branch, because we need this function for the
Ruby bindings. Your code is very easy to read, follows all of our
project conventions, and of course works perfectly. Great work!
Cheers,
David
--
David James -- http://www.cs.toronto.edu/~james
Re: [PATCH] dup function for svn_info_t
Posted by David James <ja...@gmail.com>.
On 10 Oct 2005 16:22:12 -0500, kfogel@collab.net <kf...@collab.net> wrote:
> Chris, this seems not to have made it into Subversion. Do you not
> need it anymore, or do you still want it but just were waiting for
> someone to commit it?
>
> -Karl
This patch looks perfect to me. In the Python bindings, we don't wrap
svn_client_info yet, but when we do, we'll need the svn_info_dup
function.
If you are also happy with the patch, please commit it.
Thanks!
David
>
> "Peter N. Lundblad" <pe...@famlundblad.se> writes:
> > On Mon, 13 Jun 2005, Chris Foote wrote:
> >
> > > Branko Čibej wrote:
> > > > Chris Foote wrote:
> > > >
> > > > >[LOG]
> > > > >
> > > > >Add a function to duplicate the svn_info_t structure.
> > > > >
> > > > >* subversion/include/svn_client.h
> > > > > (svn_info_t): Add note to update svn_info_dup when extending the struct.
> > > > > (svn_info_dup): New function to duplicate an svn_info_t.
> > > > >
> > > > >* subversion/libsvn_client/info.c
> > > > > (svn_info_dup): Ditto.
> > > > >
> > > > >
> > > > Why add a public function that's not used anywhere?
> > > >
> > > So that clients (i.e. one I'm writing) can create safe copies of the struct
> > > that are allocated in a different pool from the one that was used when
> > > calling svn_client_info.
> > >
> >
> > I agree. The struct might be extended, so to be safe, you need such a
> > function.
> > > > >+svn_info_t *
> > > > >+svn_info_dup (const svn_info_t *info, apr_pool_t *pool)
> > > > >+{
> > > > >+ svn_info_t *dupinfo = apr_pcalloc (pool, sizeof(*dupinfo));
> > > > >+
> > > > >+ /* Perform a trivial copy ... */
> > > > >+ *dupinfo = *info;
> > > > >
> > > > >
> > > > Since you're copying the struct anyway, there's no sense in using
> > > > apr_pcalloc.
> > > True, although this is what svn_wc_entry_dup uses.
> > >
> > I don't mind changing that too, even if this is really cycle-counting if
> > anything...
> >
> > Regards,
> > //Peter
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> > For additional commands, e-mail: dev-help@subversion.tigris.org
> >
>
> --
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
--
David James -- http://www.cs.toronto.edu/~james
Re: [PATCH] dup function for svn_info_t
Posted by Julian Foad <ju...@btopenworld.com>.
> "Peter N. Lundblad" <pe...@famlundblad.se> writes:
>>On Mon, 13 Jun 2005, Chris Foote wrote:
>>>Branko Čibej wrote:
>>
>>>>>+svn_info_t *
>>>>>+svn_info_dup (const svn_info_t *info, apr_pool_t *pool)
>>>>>+{
>>>>>+ svn_info_t *dupinfo = apr_pcalloc (pool, sizeof(*dupinfo));
>>>>>+
>>>>>+ /* Perform a trivial copy ... */
>>>>>+ *dupinfo = *info;
>>>>
>>>>Since you're copying the struct anyway, there's no sense in using
>>>>apr_pcalloc.
>>>
>>>True, although this is what svn_wc_entry_dup uses.
>>
>>I don't mind changing that too, even if this is really cycle-counting if
>>anything...
It's not cycle counting, because I don't care whether apr_palloc is faster than
apr_pcalloc. The reason to use apr_palloc is code clarity. It avoids the
reader wasting time checking: "This code seems to initialize the memory twice;
am I misunderstanding part of it? apr_pcalloc... checking... yes, that really
does initialise it. The copy "*dupinfo = *info"... checking... yes, that
really is copying the whole size of the structure, so that does too. Oh well."
svn_wc_entry_dup() fixed in r16647. Thanks for pointing it out.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org