Re: svn commit: rev 3790 - in trunk/subversion: include libsvn_subr tests/libsvn_subr

[Greg Stein <gs...@lyra.org>]

Heh. My previous comments don't really apply if you're reading from a
stringbuf, since you properly don't want to increment buf->data.

Personally, I would have created two stream types. One for reading, and one
for writing, rather than mixing them like this. With this mixed version, you
*have* to have a stringbuf. And a stringbuf's data is non-const, so you're
going to have a hard time reading from a constant string. Not to mention
that a stringbuf structure strictly needs a pool value, etc etc.

On Thu, Nov 14, 2002 at 03:30:20PM -0600, cmpilato@tigris.org wrote:
>...
> +++ trunk/subversion/include/svn_io.h	Thu Nov 14 15:29:58 2002
>...
> +/* Return a generic stream connected to STR.  Allocate the stream in
> +   POOL.  When used as readable stream, STR should contain the data
> +   you wish to read.  When used a writable stream, STR should be
> +   non-NULL, initialized by the stringbuf creation function of your
> +   choice, and writes to this stream will append data to STR.  */

STR should *always* be non-NULL. It isn't an optinal parameter, so that
phrasing is a bit weird.

>...
> +++ trunk/subversion/libsvn_subr/io.c	Thu Nov 14 15:30:02 2002
>...
> +  if (! btn->str)
> +    {
> +      *len = 0;
> +      return SVN_NO_ERROR;
> +    }
>...
> +  if (! btn->str)
> +    {
> +      *len = 0;
> +      return svn_error_create (SVN_ERR_INCORRECT_PARAMS, 0, NULL,
> +                               "No receiving string available");
> +    }

Both of these are rather non-sensical. btn->str should always exist. As with
any of our code, just rip this out. If some idiot passes a NULL str, then
they'll get a segfault right quick, so the failure mode is quite fine.

>...
> +svn_stream_from_stringbuf (svn_stringbuf_t *str,
> +                           apr_pool_t *pool)
>  {
>    svn_stream_t *stream;
>    struct string_stream_baton *baton;
>  
> -  if (! (data && len))
> +  if (! str)
>      return svn_stream_empty (pool);

Eh? Why bother. If the user has a NULL pointer, they should call
svn_stream_empty() themselves. Keep the interface as requiring a pointer.
(not to mention this behavior wasn't doc'd in the header)

>...

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/

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