On Thu, 2009-01-22 at 15:49 +0000, Julian Foad wrote:
> 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.
(I mean it's not a robust and good practice. In this particular
instance, the change is safe because null pointers are all-zeros on all
architectures to which we might want to be portable, and SVN_NO_ERROR is
already required to be zero.)
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1043272
Received on 2009-01-22 16:57:29 CET