{"thread":{"id":"27538","subject":"RFC: C code cleanup","startedAt":"2011-06-03T03:32:13Z","lastAt":"2011-06-03T23:49:15Z","messageCount":4,"participants":["Andy Lester","Drew Northup","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"169240","messageId":"3ECEA53B-C82C-4F3D-9E40-1D81EC17682E@petdance.com","threadId":"27538","inReplyTo":null,"subject":"RFC: C code cleanup","fromName":"Andy Lester","fromEmail":"andy@petdance.com","sentAt":"2011-06-03T03:32:13Z","receivedAt":"2011-06-03T03:32:13Z","isPatch":false,"sender":{"key":"andy@petdance.com","avatar":"https://gravatar.com/avatar/997ccaea115635c2be5051f455bbe6287c0f13aa59c96e1d6eb21ba3589239fb?d=mp&s=160"},"body":"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.\n\nLast 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.\n\nFollowing 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.)\n\nSummary:\n* Removed an unused argument from a static function.\n* Modify local pointers to not cast away constness  \n* Remove a test to see if either of two size_t vars are less than zero, which can never happen.\n* Make two mmfile_t * function pointers be const mmfile_t *\n* Declare three local variables to be const.\n\nI welcome your comments/suggestions/requests.\n\nxoxo,\nAndy\n\ncommit 2c527e65952ad6aeff4e58270de545724aea8785\nAuthor: Andy Lester <andy@petdance.com>\nDate:   Thu Jun 2 09:28:35 2011 -0500\n\n    Removed unused arg from parse_dates()\n\ndiff --git a/test-date.c b/test-date.c\nindex 6bcd5b0..42642ed 100644\n--- a/test-date.c\n+++ b/test-date.c\n@@ -16,7 +16,7 @@ static void show_dates(char **argv, struct timeval *now)\n \t}\n }\n \n-static void parse_dates(char **argv, struct timeval *now)\n+static void parse_dates(char **argv)\n {\n \tfor (; *argv; argv++) {\n \t\tchar result[100];\n@@ -61,7 +61,7 @@ int main(int argc, char **argv)\n \tif (!strcmp(*argv, \"show\"))\n \t\tshow_dates(argv+1, &now);\n \telse if (!strcmp(*argv, \"parse\"))\n-\t\tparse_dates(argv+1, &now);\n+\t\tparse_dates(argv+1);\n \telse if (!strcmp(*argv, \"approxidate\"))\n \t\tparse_approxidate(argv+1, &now);\n \telse\n\ncommit 6c822a7ee8885593f5338cb086d32daa779cb926\nAuthor: Andy Lester <andy@petdance.com>\nDate:   Wed Jun 1 23:29:50 2011 -0500\n\n    Change pointers in compare_pt() to be const-correct\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 66fc291..2983d5d 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -184,8 +184,8 @@ struct possible_tag {\n \n static int compare_pt(const void *a_, const void *b_)\n {\n-\tstruct possible_tag *a = (struct possible_tag *)a_;\n-\tstruct possible_tag *b = (struct possible_tag *)b_;\n+\tconst struct possible_tag *a = (const struct possible_tag *)a_;\n+\tconst struct possible_tag *b = (const struct possible_tag *)b_;\n \tif (a->depth != b->depth)\n \t\treturn a->depth - b->depth;\n \tif (a->found_order != b->found_order)\n\ncommit ed2a1e246abab43033351c14603d4ff8a5c3a5ad\nAuthor: Andy Lester <andy@petdance.com>\nDate:   Wed Jun 1 23:17:52 2011 -0500\n\n    removed unused sha1 argument from cat_one_file()\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 94632db..389faaa 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -15,7 +15,7 @@\n #define BATCH 1\n #define BATCH_CHECK 2\n \n-static void pprint_tag(const unsigned char *sha1, const char *buf, unsigned long size)\n+static void pprint_tag(const char *buf, unsigned long size)\n {\n \t/* the parser in tag.c is useless here. */\n \tconst char *endp = buf + size;\n@@ -131,7 +131,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n \t\tif (!buf)\n \t\t\tdie(\"Cannot read object %s\", obj_name);\n \t\tif (type == OBJ_TAG) {\n-\t\t\tpprint_tag(sha1, buf, size);\n+\t\t\tpprint_tag(buf, size);\n \t\t\treturn 0;\n \t\t}\n \n\ncommit 151ad9c45f08aa81598664e6e198af881fe52b77\nAuthor: Andy Lester <andy@petdance.com>\nDate:   Wed Jun 1 23:16:10 2011 -0500\n\n    Removed unnecessary test in fuzzy_matchlines(). size_t can never be negative\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 530d4bb..7e6fa4d 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -250,9 +250,6 @@ static int fuzzy_matchlines(const char *s1, size_t n1,\n \tconst char *last2 = s2 + n2 - 1;\n \tint result = 0;\n \n-\tif (n1 < 0 || n2 < 0)\n-\t\treturn 0;\n-\n \t/* ignore line endings */\n \twhile ((*last1 == '\\r') || (*last1 == '\\n'))\n \t\tlast1--;\n\ncommit 01659f145328103383d48adadee1afbb3e6b2d5d\nAuthor: Andy Lester <andy@petdance.com>\nDate:   Wed Jun 1 22:48:48 2011 -0500\n\n    mmfile_t arguments to count_trailing_blank() and check_blank_at_eof() can be const *\n\ndiff --git a/diff.c b/diff.c\nindex be3d19d..13e79af 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -318,7 +318,7 @@ static unsigned long diff_filespec_size(struct diff_filespec *one)\n \treturn one->size;\n }\n \n-static int count_trailing_blank(mmfile_t *mf, unsigned ws_rule)\n+static int count_trailing_blank(const mmfile_t *mf, unsigned ws_rule)\n {\n \tconst char *ptr = mf->ptr;\n \tconst long size = mf->size;\n@@ -344,7 +344,7 @@ static int count_trailing_blank(mmfile_t *mf, unsigned ws_rule)\n \treturn cnt;\n }\n \n-static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,\n+static void check_blank_at_eof(const mmfile_t *mf1, const mmfile_t *mf2,\n \t\t\t       struct emit_callback *ecbdata)\n {\n \tint l1, l2, at;\n\ncommit c2042e0bc6be027fc6036283b81ef640c1d37b36\nAuthor: Andy Lester <andy@petdance.com>\nDate:   Wed Jun 1 22:41:22 2011 -0500\n\n    consted three local vars in count_trailing_blank()\n\ndiff --git a/diff.c b/diff.c\nindex 8f4815b..be3d19d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -320,8 +320,8 @@ static unsigned long diff_filespec_size(struct diff_filespec *one)\n \n static int count_trailing_blank(mmfile_t *mf, unsigned ws_rule)\n {\n-\tchar *ptr = mf->ptr;\n-\tlong size = mf->size;\n+\tconst char *ptr = mf->ptr;\n+\tconst long size = mf->size;\n \tint cnt = 0;\n \n \tif (!size)\n@@ -332,7 +332,7 @@ static int count_trailing_blank(mmfile_t *mf, unsigned ws_rule)\n \telse\n \t\tptr--; /* skip the last LF */\n \twhile (mf->ptr < ptr) {\n-\t\tchar *prev_eol;\n+\t\tconst char *prev_eol;\n \t\tfor (prev_eol = ptr; mf->ptr <= prev_eol; prev_eol--)\n \t\t\tif (*prev_eol == '\\n')\n \t\t\t\tbreak;\n\n--\nAndy Lester => andy@petdance.com => www.techworklove.com => AIM:petdance\n"},{"id":"169256","messageId":"1307116956.4051.20.camel@drew-northup.unet.maine.edu","threadId":"27538","inReplyTo":"3ECEA53B-C82C-4F3D-9E40-1D81EC17682E@petdance.com","subject":"Re: RFC: C code cleanup","fromName":"Drew Northup","fromEmail":"drew.northup@maine.edu","sentAt":"2011-06-03T16:02:36Z","receivedAt":"2011-06-03T16:02:36Z","isPatch":false,"sender":{"key":"drew.northup@maine.edu","avatar":"https://avatars.githubusercontent.com/u/18331571?v=4"},"body":"\nOn Thu, 2011-06-02 at 22:32 -0500, Andy Lester wrote:\n> 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.\n> \n> 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.\n> \n> 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.)\n> \n> Summary:\n> * Removed an unused argument from a static function.\n> * Modify local pointers to not cast away constness  \n> * Remove a test to see if either of two size_t vars are less than zero, which can never happen.\n> * Make two mmfile_t * function pointers be const mmfile_t *\n> * Declare three local variables to be const.\n> \n> I welcome your comments/suggestions/requests.\n\n<snip>\n\nAndy,\nWhile cleanups like this\n\n>     Change pointers in compare_pt() to be const-correct\n> \n> diff --git a/builtin/describe.c b/builtin/describe.c\n...\n>  static int compare_pt(const void *a_, const void *b_)\n>  {\n> -\tstruct possible_tag *a = (struct possible_tag *)a_;\n> -\tstruct possible_tag *b = (struct possible_tag *)b_;\n> +\tconst struct possible_tag *a = (const struct possible_tag *)a_;\n> +\tconst struct possible_tag *b = (const struct possible_tag *)b_;\n\nmay be welcome, preferably if they are fixing a notable problem, changes\nlike \n\n>     Removed unused arg from parse_dates()\n> \n> diff --git a/test-date.c b/test-date.c\n...\n>  \n> -static void parse_dates(char **argv, struct timeval *now)\n> +static void parse_dates(char **argv)\n...\n>  \telse if (!strcmp(*argv, \"parse\"))\n> -\t\tparse_dates(argv+1, &now);\n> +\t\tparse_dates(argv+1);\n\nand this\n\n>     removed unused sha1 argument from cat_one_file()\n> \n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n...\n> -static void pprint_tag(const unsigned char *sha1, const char *buf, unsigned long size)\n> +static void pprint_tag(const char *buf, unsigned long size)\n...\n> -\t\t\tpprint_tag(sha1, buf, size);\n> +\t\t\tpprint_tag(buf, size);\n\nseem to fail to ask important questions. Why is the code the way that it\nis? Should we be making this change? What is the argument for making\nthis change?\n\nBefore proposing changes like this please use git blame to figure out\nwho should be brought in on the conversation for each patch. It may be\nthat way for a reason, it may be cruft, and it may be a mistake; but\nyou'll never know without asking on a case-by-case basis. (Also IIRC,\nsome patch authors are not on this list, using the output of blame helps\nbring them in at the right times.)\n\n-- \n-Drew Northup\n________________________________________________\n\"As opposed to vegetable or mineral error?\"\n-John Pescatore, SANS NewsBites Vol. 12 Num. 59\n"},{"id":"169274","messageId":"861C71D1-8F33-4DEE-B809-1E6CA178902D@petdance.com","threadId":"27538","inReplyTo":"1307116956.4051.20.camel@drew-northup.unet.maine.edu","subject":"Re: RFC: C code cleanup","fromName":"Andy Lester","fromEmail":"andy@petdance.com","sentAt":"2011-06-03T21:38:33Z","receivedAt":"2011-06-03T21:38:33Z","isPatch":false,"sender":{"key":"andy@petdance.com","avatar":"https://gravatar.com/avatar/997ccaea115635c2be5051f455bbe6287c0f13aa59c96e1d6eb21ba3589239fb?d=mp&s=160"},"body":">> -\t\t\tpprint_tag(sha1, buf, size);\n>> +\t\t\tpprint_tag(buf, size);\n> \n> seem to fail to ask important questions. Why is the code the way that it\n> is? Should we be making this change? What is the argument for making\n> this change?\n\nMaybe I should submit these as tickets somewhere?  Clearly, something is incorrect, either with missing code or too much of it.\n\nxoa\n\n--\nAndy Lester => andy@petdance.com => www.techworklove.com => AIM:petdance\n"},{"id":"169287","messageId":"BANLkTikxY8QcGO70Lg0HQAVZ9kwmsD0WF6HEoGC1bojWo4Myog@mail.gmail.com","threadId":"27538","inReplyTo":"3ECEA53B-C82C-4F3D-9E40-1D81EC17682E@petdance.com","subject":"Re: RFC: C code cleanup","fromName":"Junio C Hamano","fromEmail":"gitster-vger@pobox.com","sentAt":"2011-06-03T23:49:15Z","receivedAt":"2011-06-03T23:49:15Z","isPatch":false,"sender":{"key":"gitster-vger@pobox.com","avatar":null},"body":"Thanks. The ones I don't comment on below looked like sensible changes\n(except that their log messages need to conform to the project's\nstyle).\n\n> diff --git a/test-date.c b/test-date.c\n> index 6bcd5b0..42642ed 100644\n> --- a/test-date.c\n> +++ b/test-date.c\n> @@ -16,7 +16,7 @@ static void show_dates(char **argv, struct timeval *now)\n>        }\n>  }\n>\n> -static void parse_dates(char **argv, struct timeval *now)\n> +static void parse_dates(char **argv)\n>  {\n\nAs parse-dates mode does not handle any \"relative\" dates now, the\nparameter happens not to be used in today's code. But judging from the\ncaller and other callers in the same if/else cascade, it probably is\nbetter to leave this as-is.\n\n> @@ -61,7 +61,7 @@ int main(int argc, char **argv)\n>        if (!strcmp(*argv, \"show\"))\n>                show_dates(argv+1, &now);\n>        else if (!strcmp(*argv, \"parse\"))\n> -               parse_dates(argv+1, &now);\n> +               parse_dates(argv+1);\n>        else if (!strcmp(*argv, \"approxidate\"))\n>                parse_approxidate(argv+1, &now);\n>        else\n\n> commit 151ad9c45f08aa81598664e6e198af881fe52b77\n> Author: Andy Lester <andy@petdance.com>\n> Date:   Wed Jun 1 23:16:10 2011 -0500\n>\n>    Removed unnecessary test in fuzzy_matchlines(). size_t can never be negative\n>\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index 530d4bb..7e6fa4d 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -250,9 +250,6 @@ static int fuzzy_matchlines(const char *s1, size_t n1,\n>        const char *last2 = s2 + n2 - 1;\n>        int result = 0;\n>\n> -       if (n1 < 0 || n2 < 0)\n> -               return 0;\n> -\n\nLooks like the author wanted to assert() to make sure the length\nadjustments were not screwed up (a possible bug can be to update *.len\nby subtracting too many, causing it to wrap around to become a\nnegative value but because size_t is unsigned, this is a wrong test to\ncatch it).\n\nPerhaps casting these to ssize_t and dying with die(\"BUG: len\nadjustment error\") would be a better fix that is more faithful to the\nintention of the original.\n"}]}