On Thu, 2011-06-02 at 22:32 -0500, Andy Lester wrote: > Is the git project interested in patches to code that have no effect on the compiled code, but clean up some of the corners of the code itself? Or would it be seen as unnecessary code churn? I'd like to get some feedback before I spend much time on these kinds of cleanups. > > Last night I poked around the code tree and turned on gcc's -Wextra flag. If these sorts of cleanups are something git likes, I'd also look at running under splint (http://splint.org) to see what it might find as well. I've been doing the same over on the Perl 5 and Parrot projects for years. It lets me work on more janitorial issues and do what I can to help oft-neglected bits. > > Following are some changes I think would be useful. (I realize they are not in the official SubmittingPatches form. I'm only listing them as examples of what I think would be helpful.) > > Summary: > * Removed an unused argument from a static function. > * Modify local pointers to not cast away constness > * Remove a test to see if either of two size_t vars are less than zero, which can never happen. > * Make two mmfile_t * function pointers be const mmfile_t * > * Declare three local variables to be const. > > I welcome your comments/suggestions/requests. <snip> Andy, While cleanups like this > Change pointers in compare_pt() to be const-correct > > diff --git a/builtin/describe.c b/builtin/describe.c ... > static int compare_pt(const void *a_, const void *b_) > { > - struct possible_tag *a = (struct possible_tag *)a_; > - struct possible_tag *b = (struct possible_tag *)b_; > + const struct possible_tag *a = (const struct possible_tag *)a_; > + const struct possible_tag *b = (const struct possible_tag *)b_; may be welcome, preferably if they are fixing a notable problem, changes like > Removed unused arg from parse_dates() > > diff --git a/test-date.c b/test-date.c ... > > -static void parse_dates(char **argv, struct timeval *now) > +static void parse_dates(char **argv) ... > else if (!strcmp(*argv, "parse")) > - parse_dates(argv+1, &now); > + parse_dates(argv+1); and this > removed unused sha1 argument from cat_one_file() > > diff --git a/builtin/cat-file.c b/builtin/cat-file.c ... > -static void pprint_tag(const unsigned char *sha1, const char *buf, unsigned long size) > +static void pprint_tag(const char *buf, unsigned long size) ... > - pprint_tag(sha1, buf, size); > + pprint_tag(buf, size); seem to fail to ask important questions. Why is the code the way that it is? Should we be making this change? What is the argument for making this change? Before proposing changes like this please use git blame to figure out who should be brought in on the conversation for each patch. It may be that way for a reason, it may be cruft, and it may be a mistake; but you'll never know without asking on a case-by-case basis. (Also IIRC, some patch authors are not on this list, using the output of blame helps bring them in at the right times.) -- -Drew Northup ________________________________________________ "As opposed to vegetable or mineral error?" -John Pescatore, SANS NewsBites Vol. 12 Num. 59 -- 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