Re: C internals cleanup

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 13, 2009, 04:06 UTC
Message-ID: <7v4owtw623.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <22578EEA-DB8B-4DAF-B217-FF13DC8A3EC7@petdance.com>

Andy Lester <andy@petdance.com> writes:

Show 11 quoted lines

> I've been poking around in the source for git, and wanted to pitch in
> and clean some things up.
>
> Two biggies that tend to get overlooked: Applying the const keyword
> where possible, and localizing variables to innermost blocks.
>
> Also, want to to get a target going in the Makefile for running under
> splint.
>
> Just want to make sure my internal cleanups are not going to be seen
> as a nuisance.

I and my lieutenants are most likely polite enough to avoid using a word like nuisance, but I think you will hear words like code churn, especially if such a change affects too many places and conflicts with too many patches that are still in the queue to be merged. IOW, you need to be careful.

The general rule of thumb is to do such a clean-up before you start to work on something of substance.

Your series may look like this:

 * [Patch 1/2] hello.c: tighten constness and scope

   Many functions in this file do not have to be called from other files,
   and goodmorning() takes "char *" parameters but does not modify them in
   any way (others like goodafternoon() and goodevening() are already
   correct in this regard, both taking "const char *" parameters).

   Make all of tese functions file scope static, and tighten constness to
   the parameters to gootmorning().  By making their function signatures
   compatible, this change will help the next patch that refactors the
   three greeting functions.

 * [Patch 2/2] hello.c: refactor goodmorning(), goodafternoon() and goodnight()

   These functions do mostly the same thing; implement a common helper
   function greeting() and share code among them.

Otherwise, unless the tree is really quiet, a patch that is only clean-up and nothing else will have a high maintenance-cost vs reward ratio, and needs to be split and timed carefully. A patch that applies cleanly to master and then the result merges cleanly to both next and pu would be Ok even if you do not add any value other than code cleanliness.