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

From: David Rientjes <rientjes@google.com>
Date: Aug 14, 2006, 23:30 UTC
Message-ID: <Pine.LNX.4.63.0608141618470.21705@chino.corp.google.com>
In-Reply-To: <7vlkpqdikx.fsf@assigned-by-dhcp.cox.net>

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.

Show 25 quoted lines

>  * 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 06/28] make cmd_log_walk void
>         [PATCH 07/28] builtin-mailinfo.c cleanup
>         [PATCH 09/28] makes prune_dir void
>         [PATCH 11/28] makes append_ref and show_indepedent void
>         [PATCH 12/28] makes generate_tar void
>         [PATCH 13/28] builtin-unpack-objects.c cleanup
>         [PATCH 14/28] make do_reupdate void
>         [PATCH 16/28] daemon.c cleanup
>         [PATCH 17/28] makes diff_cache void
>         [PATCH 19/28] makes finish_pack void
>         [PATCH 20/28] makes fetch_pack void
>         [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. It also suggests you can program against a specific return value with expected results which are, in fact, true for any return. Additionally, changes in the future will be easier, in my opinion, because a void -> int change is very simple and existant calls need not be changed (their return values are discarded) so that the implementer can make the checks where necessary.

I do not understand your point about making error checking of the functions stricter without affecting the rest of the code when int returns are discarded if not assigned upon return. It would certainly be bad style to declare new error codes with a specific meaning without checking all the affected code and we certainly cannot predict this behavior now.

		David