Re: [PATCH 00/28] clean-ups of static functions and returns

From: Junio C Hamano <junkio@cox.net>
Date: Aug 15, 2006, 01:04 UTC
Message-ID: <7vsljyajxj.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.63.0608141618470.21705@chino.corp.google.com>

David Rientjes <rientjes@google.com> writes:

Show 28 quoted lines

> On Mon, 14 Aug 2006, Junio C Hamano wrote:
>
>> Interesting.  Did you use some automated tool to spot them?
>> 
>
> No, these changes are from my own personal tree that optimizes everything for 
> speed since I am working with terabytes of data.  I only submitted changes that 
> I thought would be beneficial for your project as well.
>
>>  * Making stricter error checking in the future harder.  There
>>    are three classes, but the lines between them are fuzzy.
>> 
>>         [PATCH 04/28] builtin-diff.c cleanup
>>...
>>         [PATCH 23/28] makes peek_remote void
>> 
>>    The callers of the first group check their return values, so
>>    we could make error checking of these functions stricter in
>>    the future without affecting the rest of the code.  The ones
>>    that currently die() (or usage()) could be made into more
>>    libified form to return error codes.
>> 
>>    So I do not think it is worth doing these.
>
> I disagree.  Having static functions return ints that are the
> same for _every_ code path and are checked against upon return
> is never good style.  It implies that error checking is
> already done and the return value is of importance.

I would agree with your above statement about the second group, but not the ones listed in the first group, whose callers are prepared to receive error returns. It just happens that these callees do not currently detect errors, but some of them certainly could be improved to return errors, instead of just calling die().