[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r32558 - trunk/subversion/libsvn_fs_fs

From: Karl Fogel <kfogel_at_red-bean.com>
Date: Wed, 20 Aug 2008 11:31:00 -0400

cmpilato_at_tigris.org writes:
> Log:
> Fix SEGFAULT introduced in r32545.
>
> * subversion/libsvn_fs_fs/tree.c
> (fs_node_origin_rev): Avoid dup'ing a NULL pointer.
>
> --- trunk/subversion/libsvn_fs_fs/tree.c
> +++ trunk/subversion/libsvn_fs_fs/tree.c
> @@ -3084,7 +3084,7 @@ fs_node_origin_rev(svn_revnum_t *revisio
> (which is allocated in SUBPOOL ... maybe). */
> svn_pool_clear(predidpool);
> SVN_ERR(svn_fs_fs__dag_get_predecessor_id(&pred_id, node, subpool));
> - pred_id = svn_fs_fs__id_copy(pred_id, predidpool);
> + pred_id = pred_id ? svn_fs_fs__id_copy(pred_id, predidpool) : NULL;
> }

Not a big deal, but that way of expressing the logic felt a little odd
to me. Instead, wouldn't

   if (pred_id)
     pred_id = svn_fs_fs__id_copy(pred_id, predidpool);

...be the usual way to do this?

The "foo ? bar : baz;" syntax implies that 'baz' part makes a
difference. But here it has no effect: if pred_id is NULL, then set it
to NULL :-). So I think 'if' would be more natural for cases like this.
Very minor readability boost; I don't think it's worth the code/annotate
churn to tweak it now, just pointing it out for next time.

-K

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-08-20 17:31:23 CEST

This is an archived mail posted to the Subversion Dev mailing list.