Kamesh Jayachandran wrote:
> Julian Foad wrote:
> > Author: julianfoad
> > Date: Thu Dec 18 14:34:04 2008
> > New Revision: 34827
> >
> > Log:
> > * subversion/libsvn_ra_neon/mergeinfo.c
> > (cdata_handler): Change bug-hiding "if"s into bug-revealing assertions.
> >
> > Modified:
> > trunk/subversion/libsvn_ra_neon/mergeinfo.c
> >
> > Modified: trunk/subversion/libsvn_ra_neon/mergeinfo.c
> > URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_ra_neon/mergeinfo.c?pathrev=34827&r1=34826&r2=34827
> > ==============================================================================
> > --- trunk/subversion/libsvn_ra_neon/mergeinfo.c Thu Dec 18 14:20:46 2008 (r34826)
> > +++ trunk/subversion/libsvn_ra_neon/mergeinfo.c Thu Dec 18 14:34:04 2008 (r34827)
> > @@ -133,13 +133,13 @@ cdata_handler(void *baton, int state, co
> > switch (state)
> > {
> > case ELEM_mergeinfo_path:
> > - if (mb->curr_path)
> > - svn_stringbuf_appendbytes(mb->curr_path, cdata, nlen);
> > + SVN_ERR_ASSERT(mb->curr_path);
> > + svn_stringbuf_appendbytes(mb->curr_path, cdata, nlen);
>
> I think both 'bug hiding if' and 'SVN_ERR_ASSERT' are extraneous as
> mb->curr_path will *never* be NULL as per our code i.e
> svn_ra_neon__get_mergeinfo allocates mb->curr_path to empty buffer so no
> chance of these to be NULL.
[...]
Yes, I agree that it can never be null. And that's exactly what
assertions are for: checking for things that can never happen unless
someone has introduced a bug. However, we can rely on computer hardware
to catch this kind of bug (writing to a null pointer), so in that sense
the ASSERTs are of no use at all.
SVN_ERR_ASSERT does have one benefit over hardware checks, which is that
it can return a nice error to the caller, if so configured. But we don't
want to put "SVN_ERR_ASSERT(pointer != NULL);" before every write to a
pointer, because that would massively get in the way of reading and
maintaining the code.
So I agree. Committed r34832.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=987361
Received on 2008-12-19 12:33:02 CET