Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?

-- 
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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]