Greg Stein wrote:
> They should NOT be switched over without prejudice. Total -1 to that.
>
> Review them? Sure. But I'm very much against a simple search/replace.
> That's bogus.
Indeed. Execution time is not the primary consideration for switching to
always-initialise-to-zero.
Would the execution time be negligible? Yes, almost everywhere.
Would the change prevent uninitialised fields? Fields that are not
explicitly initialized would get the value 0 rather than an
unpredictable value, and zero may be a "safe" and even the right value
in many cases, and the predictability may help debugging in cases where
it is not the right value.
Would the change reduce the likelihood of us introducing bugs? If the
programmer forgets to initialise a new field, the result would be the
same as if he had chosen that its value should be zero, so the lack of
thought is hidden and the code might work as intended... or might not.
And there's a different kind of bug if we choose not to
double-initialise fields that should be zero...
The change in r35366:
[[[
------------------------------------------------------------------------
r35366 | rhuijben | 2009-01-21 15:07:05 +0000 (Wed, 21 Jan 2009) | 9 lines
* subversion/libsvn_wc/util.c
(svn_wc_create_notify): Use apr_pcalloc and only set fields that have a
non null value.
Found by: hwright
Resolves the segfault reported in
<http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1041272>
------------------------------------------------------------------------
Index: subversion/libsvn_wc/util.c
===================================================================
--- subversion/libsvn_wc/util.c (revision 35365)
+++ subversion/libsvn_wc/util.c (revision 35366)
@@ -116,19 +116,13 @@
svn_wc_notify_action_t action,
apr_pool_t *pool)
{
- svn_wc_notify_t *ret = apr_palloc(pool, sizeof(*ret));
+ svn_wc_notify_t *ret = apr_pcalloc(pool, sizeof(*ret));
ret->path = path;
ret->action = action;
ret->kind = svn_node_unknown;
- ret->mime_type = NULL;
- ret->lock = NULL;
- ret->err = SVN_NO_ERROR;
ret->content_state = ret->prop_state = svn_wc_notify_state_unknown;
ret->lock_state = svn_wc_notify_lock_state_unknown;
ret->revision = SVN_INVALID_REVNUM;
- ret->changelist_name = NULL;
- ret->merge_range = NULL;
- ret->path_prefix = NULL;
return ret;
}
]]]
... trades one kind of potential error (forgetting to initialize a new
field) for another kind. The new code encodes the knowledge that
SVN_INVALID_REVNUM and svn_wc_notify_lock_state_unknown have non-zero
bit patterns, and that SVN_NO_ERROR has an all-zero bit pattern. This is
not robust and, I assert, not good.
- Julian
> Hyrum K. Wright wrote:
> > C. Michael Pilato wrote:
> >> Stefan Sperling wrote:
> >>> On Wed, Jan 21, 2009 at 11:11:59PM +0100, Bert Huijben wrote:
> >>>> Note that this was just a dangling pointer in a svn_wc_notify_t
> >>>> structure
> >>>> that wasn't initialized with apr_pcalloc as I expected, but with
> >>>> apr_palloc.
> >>>> See r35366.
> >>>
> >>> Maybe we should also do an apr_palloc -> apr_pcalloc sweep?
> >>
> >> +1. I know, I know, I know that careful coders shouldn't need to force a
> >> memset(0) on all their allocations. But sometimes we just aren't
> >> perfectly
> >> careful coders, and the cost of such runtime crutches approaches zero
> >> daily
> >> (and has already, in my opinion, dipped well below the threshold of
> >> user-visibility).
> >
> > FWIW, there are 403 locations of apr_palloc() in our code which will need to
> > be switched over.
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1043264
Received on 2009-01-22 16:49:23 CET