RFC: C code cleanup

RFC: C code cleanup

[Andy Lester]

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.

xoxo,
Andy

commit 2c527e65952ad6aeff4e58270de545724aea8785
Author: Andy Lester <andy@petdance.com>
Date:   Thu Jun 2 09:28:35 2011 -0500

    Removed unused arg from parse_dates()

diff --git a/test-date.c b/test-date.c
index 6bcd5b0..42642ed 100644
--- a/test-date.c
+++ b/test-date.c
@@ -16,7 +16,7 @@ static void show_dates(char **argv, struct timeval *now)
 	}
 }
 
-static void parse_dates(char **argv, struct timeval *now)
+static void parse_dates(char **argv)
 {
 	for (; *argv; argv++) {
 		char result[100];
@@ -61,7 +61,7 @@ int main(int argc, char **argv)
 	if (!strcmp(*argv, "show"))
 		show_dates(argv+1, &now);
 	else if (!strcmp(*argv, "parse"))
-		parse_dates(argv+1, &now);
+		parse_dates(argv+1);
 	else if (!strcmp(*argv, "approxidate"))
 		parse_approxidate(argv+1, &now);
 	else

commit 6c822a7ee8885593f5338cb086d32daa779cb926
Author: Andy Lester <andy@petdance.com>
Date:   Wed Jun 1 23:29:50 2011 -0500

    Change pointers in compare_pt() to be const-correct

diff --git a/builtin/describe.c b/builtin/describe.c
index 66fc291..2983d5d 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -184,8 +184,8 @@ struct possible_tag {
 
 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_;
 	if (a->depth != b->depth)
 		return a->depth - b->depth;
 	if (a->found_order != b->found_order)

commit ed2a1e246abab43033351c14603d4ff8a5c3a5ad
Author: Andy Lester <andy@petdance.com>
Date:   Wed Jun 1 23:17:52 2011 -0500

    removed unused sha1 argument from cat_one_file()

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 94632db..389faaa 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -15,7 +15,7 @@
 #define BATCH 1
 #define BATCH_CHECK 2
 
-static void pprint_tag(const unsigned char *sha1, const char *buf, unsigned long size)
+static void pprint_tag(const char *buf, unsigned long size)
 {
 	/* the parser in tag.c is useless here. */
 	const char *endp = buf + size;
@@ -131,7 +131,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
 		if (!buf)
 			die("Cannot read object %s", obj_name);
 		if (type == OBJ_TAG) {
-			pprint_tag(sha1, buf, size);
+			pprint_tag(buf, size);
 			return 0;
 		}
 

commit 151ad9c45f08aa81598664e6e198af881fe52b77
Author: Andy Lester <andy@petdance.com>
Date:   Wed Jun 1 23:16:10 2011 -0500

    Removed unnecessary test in fuzzy_matchlines(). size_t can never be negative

diff --git a/builtin/apply.c b/builtin/apply.c
index 530d4bb..7e6fa4d 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -250,9 +250,6 @@ static int fuzzy_matchlines(const char *s1, size_t n1,
 	const char *last2 = s2 + n2 - 1;
 	int result = 0;
 
-	if (n1 < 0 || n2 < 0)
-		return 0;
-
 	/* ignore line endings */
 	while ((*last1 == '\r') || (*last1 == '\n'))
 		last1--;

commit 01659f145328103383d48adadee1afbb3e6b2d5d
Author: Andy Lester <andy@petdance.com>
Date:   Wed Jun 1 22:48:48 2011 -0500

    mmfile_t arguments to count_trailing_blank() and check_blank_at_eof() can be const *

diff --git a/diff.c b/diff.c
index be3d19d..13e79af 100644
--- a/diff.c
+++ b/diff.c
@@ -318,7 +318,7 @@ static unsigned long diff_filespec_size(struct diff_filespec *one)
 	return one->size;
 }
 
-static int count_trailing_blank(mmfile_t *mf, unsigned ws_rule)
+static int count_trailing_blank(const mmfile_t *mf, unsigned ws_rule)
 {
 	const char *ptr = mf->ptr;
 	const long size = mf->size;
@@ -344,7 +344,7 @@ static int count_trailing_blank(mmfile_t *mf, unsigned ws_rule)
 	return cnt;
 }
 
-static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,
+static void check_blank_at_eof(const mmfile_t *mf1, const mmfile_t *mf2,
 			       struct emit_callback *ecbdata)
 {
 	int l1, l2, at;

commit c2042e0bc6be027fc6036283b81ef640c1d37b36
Author: Andy Lester <andy@petdance.com>
Date:   Wed Jun 1 22:41:22 2011 -0500

    consted three local vars in count_trailing_blank()

diff --git a/diff.c b/diff.c
index 8f4815b..be3d19d 100644
--- a/diff.c
+++ b/diff.c
@@ -320,8 +320,8 @@ static unsigned long diff_filespec_size(struct diff_filespec *one)
 
 static int count_trailing_blank(mmfile_t *mf, unsigned ws_rule)
 {
-	char *ptr = mf->ptr;
-	long size = mf->size;
+	const char *ptr = mf->ptr;
+	const long size = mf->size;
 	int cnt = 0;
 
 	if (!size)
@@ -332,7 +332,7 @@ static int count_trailing_blank(mmfile_t *mf, unsigned ws_rule)
 	else
 		ptr--; /* skip the last LF */
 	while (mf->ptr < ptr) {
-		char *prev_eol;
+		const char *prev_eol;
 		for (prev_eol = ptr; mf->ptr <= prev_eol; prev_eol--)
 			if (*prev_eol == '\n')
 				break;

--
Andy Lester => andy@petdance.com => www.techworklove.com => AIM:petdance

Re: RFC: C code cleanup

[Drew Northup]

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

Re: RFC: C code cleanup

[Andy Lester]

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

Maybe I should submit these as tickets somewhere?  Clearly, something is incorrect, either with missing code or too much of it.

xoa

--
Andy Lester => andy@petdance.com => www.techworklove.com => AIM:petdance

Re: RFC: C code cleanup

[Junio C Hamano]

Thanks. The ones I don't comment on below looked like sensible changes
(except that their log messages need to conform to the project's
style).

> diff --git a/test-date.c b/test-date.c
> index 6bcd5b0..42642ed 100644
> --- a/test-date.c
> +++ b/test-date.c
> @@ -16,7 +16,7 @@ static void show_dates(char **argv, struct timeval *now)
>        }
>  }
>
> -static void parse_dates(char **argv, struct timeval *now)
> +static void parse_dates(char **argv)
>  {

As parse-dates mode does not handle any "relative" dates now, the
parameter happens not to be used in today's code. But judging from the
caller and other callers in the same if/else cascade, it probably is
better to leave this as-is.

> @@ -61,7 +61,7 @@ int main(int argc, char **argv)
>        if (!strcmp(*argv, "show"))
>                show_dates(argv+1, &now);
>        else if (!strcmp(*argv, "parse"))
> -               parse_dates(argv+1, &now);
> +               parse_dates(argv+1);
>        else if (!strcmp(*argv, "approxidate"))
>                parse_approxidate(argv+1, &now);
>        else

> commit 151ad9c45f08aa81598664e6e198af881fe52b77
> Author: Andy Lester <andy@petdance.com>
> Date:   Wed Jun 1 23:16:10 2011 -0500
>
>    Removed unnecessary test in fuzzy_matchlines(). size_t can never be negative
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 530d4bb..7e6fa4d 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -250,9 +250,6 @@ static int fuzzy_matchlines(const char *s1, size_t n1,
>        const char *last2 = s2 + n2 - 1;
>        int result = 0;
>
> -       if (n1 < 0 || n2 < 0)
> -               return 0;
> -

Looks like the author wanted to assert() to make sure the length
adjustments were not screwed up (a possible bug can be to update *.len
by subtracting too many, causing it to wrap around to become a
negative value but because size_t is unsigned, this is a wrong test to
catch it).

Perhaps casting these to ssize_t and dying with die("BUG: len
adjustment error") would be a better fix that is more faithful to the
intention of the original.