Login | Register
My pages Projects Community openCollabNet

Discussions > dev [DISABLED] > Re: svn commit: r34827 - trunk/subversion/libsvn_ra_neon

subversion
Discussion topic

There will be a brief maintenance window every Friday at 17:00 Pacific.
For further details, see CollabNet's maintenance and upgrade policy.

Back to topic list

Re: svn commit: r34827 - trunk/subversion/libsvn_ra_neon

Author julianfoad
Full name Julian Foad
Date 2008-12-19 03:32:48 PST
Message 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/lib​svn_ra_neon/mergeinf​o.c
> >
> > Modified: trunk/subversion/lib​svn_ra_neon/mergeinf​o.c
> > URL: http://svn.collab.ne​t/viewvc/svn/trunk/s​ubversion/libsvn_ra_​neon/mergeinfo.c?pat​hrev=34827&r1=34​826&r2=34827
> > ====================​====================​====================​==================
> > --- trunk/subversion/lib​svn_ra_neon/mergeinf​o.c Thu Dec 18 14:20:46 2008 (r34826)
> > +++ trunk/subversion/lib​svn_ra_neon/mergeinf​o.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_append​bytes(mb->curr_pa​th, cdata, nlen);
> > + SVN_ERR_ASSERT(mb-​>curr_path);
> > + svn_stringbuf_append​bytes(mb->curr_pa​th, 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

« Previous message in topic | 2 of 2 | Next message in topic »

Messages

Show all messages in topic

Subject
Author
Full name
Date posted
+
Re: svn commit: r34827 - trunk/subversion/libsvn_ra_neon kameshj Kamesh Jayachandran 2008-12-19 02:17:40 PST
     Re: svn commit: r34827 - trunk/subversion/libsvn_ra_neon julianfoad Julian Foad 2008-12-19 03:32:48 PST
Messages per page: