Re: svn commit: r1302682 - in /subversion/trunk/subversion/libsvn_ra_serf: ra_serf.h sb_bucket.c serf.c update.c util.c

On Thu, Mar 22, 2012 at 04:33, Julian Foad <julianfoad_at_btopenworld.com> wrote:
> Greg Stein wrote:
>> Daniel Shahaf wrote:
>>>  gstein_at_apache.org wrote:
>>>>  +svn_ra_serf__copy_into_spillbuf(svn_spillbuf_t **spillbuf,
>>>>  +                                serf_bucket_t *bkt,
>>>>  +                                apr_pool_t *result_pool,
>>>>  +                                apr_pool_t *scratch_pool)
>>>>  +{
>>>>  +      status = serf_bucket_read(bkt, SERF_READ_ALL_AVAIL, &data, &len);
>>>>  +
>>>>  +      /* ### we should throw an error, if the bucket does.  */
>>>>  +      SVN_ERR_ASSERT(status == APR_SUCCESS || status == APR_EOF);
>>>
>>>  Can we please avoid these in new code?
>>
>> Why?
>
> Hi Greg.  I'm puzzled by your response.  You wrote "### we should throw an error" -- and now you're asking Daniel why?

Daniel removed one of these ASSERT uses a day or two ago. My
assumption was that he was referring to that, rather than the ###.

>> I see no problem here: it will just bail out with an error.
>
> Not true in general; only if the application writer explicitly sets the malfunction handler to do so.

I'm assuming that all applications *do* set it to return. We use
SVN_ERR_ASSERT() liberally upon that assumption.

> Anyway, the point is not only about how the application exits, but about writing code that says what we mean.

I have no idea what you mean here.

Cheers,
-g
Received on 2012-03-22 18:20:14 CET