On Sun, Jun 2, 2013 at 6:06 PM, René Scharfe
<rene.scharfe@xxxxxxxxxxxxxx> wrote:
> Am 03.06.2013 00:38, schrieb Felipe Contreras:
>
>> On Sun, Jun 2, 2013 at 3:26 PM, René Scharfe
>> <rene.scharfe@xxxxxxxxxxxxxx> wrote:
>>>
>>> Am 02.06.2013 19:59, schrieb Felipe Contreras:
>>>
>>>> On Sun, Jun 2, 2013 at 12:54 PM, René Scharfe
>>>> <rene.scharfe@xxxxxxxxxxxxxx> wrote:
>>>>>
>>>>>
>>>>> Am 02.06.2013 19:25, schrieb Felipe Contreras:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Sun, Jun 2, 2013 at 10:46 AM, René Scharfe
>>>>>> <rene.scharfe@xxxxxxxxxxxxxx> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> + for (i = 0; i < n; i++) {
>>>>>>> + struct cache_entry *ce = src[i + o->merge];
>>>>>>> + if (ce != o->df_conflict_entry)
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> It's possible that ce is NULL, but you didn't add that check because
>>>>>> free(NULL) still works? Or because ce cannot be NULL?
>>>>>>
>>>>>> If it's the former, I think it's clearer if we check that ce is not
>>>>>> NULL either way.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> It is NULL if one tree misses an entry (e.g. a new or removed file).
>>>>> free
>>>>> handles NULL and we generally avoid duplicating its NULL-check.
>>>>
>>>>
>>>>
>>>> Yeah, but I can see somebody adding code inside that 'if' clause to
>>>> print the cache entry, and see a crash only to wonder what's going on.
>>>> And to save what? 5 characters?
>>>
>>>
>>>
>>> The person adding code that depends on ce not being NULL needs to add
>>> that
>>> check as well. Let's not worry too much about future changes that may or
>>> (more likely IMHO) may not be done. The test suite covers this case
>>> multiple times, so such a mistake doesn't have a realistic chance to hit
>>> master.
>>
>>
>> What do we gain by not doing this? 5 less characters?
>
>
> By following the convention of not checking for NULL when freeing,
That's not what I asked.
I didn't say we should do 'if (ce) free(ce);' instead of 'free(ce);' I
said we should do 'if (cd && ce != o->df_conflict_entry)' instead of
'if (ce != o->df_conflict_entry)'.
There's no reason not to.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html