Re: svn commit: rev 1968 - trunk/subversion/libsvn_fs

[Karl Fogel <kf...@newton.ch.collab.net>]

philip@tigris.org writes:
> Log:
> Fix issue 705.  Don't use the trail pool for the returned conflict path.
> 
> * subversion/libsvn_fs/tree.c
>   (merge): Change parameter to svn_stringbuf_t*, and set it using
>   svn_stringbuf_set.
>   (merge_args): Make conflict an svn_stringbuf_t*.
>   (svn_fs_commit_txn, svn_fs_merge_txn): Initialise merge_args.conflict
>   using svn_stringbuf_create.

Hrm.  Good that the bug is fixed, but...

I think it would be better to just pass in a pool, and promise to
allocate *CONFLICT_P in that pool, than to change CONFLICT_P's type to
svn_stringbuf_t *.

It may seem simpler to just change the type of an argument than to add
an argument, but in reality, you've *still* added the pool argument --
only callers can't pass it directly, they have to wrap it up in a
stringbuf package and pass that instead :-).

Easier just to let the callers pass the pool through, and keep the
clarity of `const char *' semantics (a stringbuf implies the data
might get modified afterwards), and also the clarity of explicit
lifetime documentation for CONFLICT_P.

-K

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: rev 1968 - trunk/subversion/libsvn_fs

[Karl Fogel <kf...@newton.ch.collab.net>]

Philip Martin <ph...@codematters.co.uk> writes:
> My view is that wrapping the pool is desirable.  CONFLICT_P should not
> be allocated from any other pool, and the pool should not be used for
> any other allocation.  Passing a single variable helps to enforce
> these restrictions.  Passing two separate variables doesn't seem the
> right thing to do.
> 
> "Yes, I know I'm giving you this pool, but don't use it for any of
>  your general allocations.  You must make other arrangements for
>  those, this pool is only to be used for the conflict path."

Hmm.  That's an argument I hadn't thought of.  Yeah, okay, uh, +0.

:-)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: rev 1968 - trunk/subversion/libsvn_fs

[Philip Martin <ph...@codematters.co.uk>]

Karl Fogel <kf...@newton.ch.collab.net> writes:

> philip@tigris.org writes:
> > Log:
> > Fix issue 705.  Don't use the trail pool for the returned conflict path.
> > 
> > * subversion/libsvn_fs/tree.c
> >   (merge): Change parameter to svn_stringbuf_t*, and set it using
> >   svn_stringbuf_set.
> >   (merge_args): Make conflict an svn_stringbuf_t*.
> >   (svn_fs_commit_txn, svn_fs_merge_txn): Initialise merge_args.conflict
> >   using svn_stringbuf_create.
> 
> Hrm.  Good that the bug is fixed, but...
> 
> I think it would be better to just pass in a pool, and promise to
> allocate *CONFLICT_P in that pool, than to change CONFLICT_P's type to
> svn_stringbuf_t *.

I did consider that.

> 
> It may seem simpler to just change the type of an argument than to add
> an argument, but in reality, you've *still* added the pool argument --
> only callers can't pass it directly, they have to wrap it up in a
> stringbuf package and pass that instead :-).

My view is that wrapping the pool is desirable.  CONFLICT_P should not
be allocated from any other pool, and the pool should not be used for
any other allocation.  Passing a single variable helps to enforce
these restrictions.  Passing two separate variables doesn't seem the
right thing to do.

"Yes, I know I'm giving you this pool, but don't use it for any of
 your general allocations.  You must make other arrangements for
 those, this pool is only to be used for the conflict path."

-- 
Philip

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org