On 12/18/05, Peter N. Lundblad <peter@famlundblad.se> wrote:
> On Sun, 18 Dec 2005 dionisos@tigris.org wrote:
>
> > Log:
> > Implement an apr-file-forwarding stream which adheres to the resource
> > ownership model recently discussed on the list.
> >
> > Note:
> > This commit does not deprecate svn_stream_from_aprfile, because
> > that routine is the apr file equivalent of svn_stream_disown().
> >
> > * subversion/include/svn_io.h
> > * subversion/libsvn_subr/stream.c
> > (svn_stream_from_aprfile2): New. Function to create a stream which
> > closes the underlying apr file.
> >
> Please don't name the new function svn_stream_from_aprfile2! How is one
> supposed to understand which function is which? Either keep the name and
> deprecate the old function and use svn_stream_disown when that semantic is
> wanted, or add a boolean flag to the new function and deprecate the old
> one.
Generally, that should be ok, since we - normally - should pass around
streams instead of filehandles. Streams are satisfying for by far the
most uses we have. But: using svn_stream_disown and
svn_stream_from_aprfile2 is not exactly the same semantic as th one
provided by svn_stream_from_aprfile, because the disowning is one
level lower in the _aprfile() case.
So, I was actually pondering whether to send a mail to propose
deprecating svn_stream_from_aprfile() and creating (the otherwise
equal) svn_stream_from_aprfile_disown().
I really would dislike to create lots of stream constructors with a
disown argument or with _disown() suffixes.
> > Modified: trunk/subversion/include/svn_io.h
> > Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/include/svn_io.h?rev=17834&p1=trunk/subversion/include/svn_io.h&p2=trunk/subversion/include/svn_io.h&r1=17833&r2=17834
> > ==============================================================================
> > --- trunk/subversion/include/svn_io.h (original)
> > +++ trunk/subversion/include/svn_io.h Sun Dec 18 07:35:35 2005
> > @@ -559,13 +559,26 @@
> > */
> > svn_stream_t *svn_stream_disown (svn_stream_t *stream, apr_pool_t *pool);
> >
> > -/** Convenience function for creating streams which operate on APR
> > - * files. For convenience, if @a file is NULL then svn_stream_empty(pool)
> > - * is returned. Note that the stream returned by these operations is not
> > - * considered to "own" the underlying file, meaning that svn_stream_close()
> > - * on the stream will not close the file.
> > +/** Function to create streams which operate on APR * files.
>
> These "Function to" sound odd to me; that shouldn't be part of the
> docstring. (Same below.)
Ok. Will fix.
bye,
Erik.
Received on Sun Dec 18 22:47:48 2005