Karl Fogel wrote:
> 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.
Noted.
--
C. Michael Pilato <cmpilato_at_collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Received on 2008-08-20 18:22:27 CEST