{"thread":{"id":"41458","subject":"[PATCH] GSoC Micoproject: Hunt down signed int flags","startedAt":"2016-02-21T11:13:09Z","lastAt":"2016-02-22T09:32:55Z","messageCount":4,"participants":["Saurav Sachidanand","Moritz Neeb","Eric Sunshine","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"278773","messageId":"1456053189-5221-1-git-send-email-sauravsachidanand@gmail.com","threadId":"41458","inReplyTo":null,"subject":"[PATCH] GSoC Micoproject: Hunt down signed int flags","fromName":"Saurav Sachidanand","fromEmail":"sauravsachidanand@gmail.com","sentAt":"2016-02-21T11:13:09Z","receivedAt":"2016-02-21T11:13:09Z","isPatch":true,"sender":{"key":"sauravsachidanand@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12742708?v=4"},"body":"This is patch is for a suggested micro project for GSoC 2016; namely,\nthat of searching for a field of a struct that is of signed integral\ntype and used as a collection of multiple bits, and converting it to\nan unsigned type if the MSB isn’t used in any special way.\n\nTwo structs, `pattern` defined in attr.c and `exclude` defined in dir.h,\nhave a `flags` field of signed int type. The fields of both structs take\non values from the same set of positive integers {1, 4, 8, 16},\nenumerated through the marco EXC_FLAG_*. `pattern` is used only in attr.c,\nand `exclude` is used only in builtin/check-ignore.c and dir.c, and in\nthose files, either, the value of `flags` is checked using the `&` operator\n(e.g.: flags & EXC_FLAG_NODIR), or the value of `flags` is first set to 0\nand then set to any one of {1, 4, 8, 16} using the `|=` operator\n(e.g.: flags |= EXC_FLAG_NODIR). And, so it does not appear that the MSB\nof `flags` is used in any special way. Therefore, I thought to change the\ntype of `flags` in the definitions of both structs to `unsigned int`.\n\nFurthermore, `flags` is passed by reference (of `pattern` in attr.c and of\n`exclude` in dir.c) to the function `parse_exclude_pattern` defined in\ndir.c, that accepts an `int *` type for `flags`. When make was run, it gave\na warning for ‘converting between pointers to integer types of different\nsign’, so I changed the type of that respective argument to `unsigned int *`.\n\nIn the end, running make to build didn’t produce any more warnings, and\nrunning make in t/ didn’t produce any breakage that wasn’t ‘#TODO known\nbreakage’.\n\nI also thought it’d be helpful to add the comment /* EXC_FLAG_* */ next\nto `flags` of `exclude`, just like it exists for `flags` of `pattern`.\n\nSigned-off-by: Saurav Sachidanand <sauravsachidanand@gmail.com>\n---\n attr.c | 2 +-\n dir.c  | 4 ++--\n dir.h  | 4 ++--\n 3 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 086c08d..874f726 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -124,7 +124,7 @@ struct pattern {\n \tconst char *pattern;\n \tint patternlen;\n \tint nowildcardlen;\n-\tint flags;\t\t/* EXC_FLAG_* */\n+\tunsigned int flags;\t\t/* EXC_FLAG_* */\n };\n \n /*\ndiff --git a/dir.c b/dir.c\nindex f0b6d0a..2d657e1 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -457,7 +457,7 @@ int no_wildcard(const char *string)\n \n void parse_exclude_pattern(const char **pattern,\n \t\t\t   int *patternlen,\n-\t\t\t   int *flags,\n+\t\t\t   unsigned int *flags,\n \t\t\t   int *nowildcardlen)\n {\n \tconst char *p = *pattern;\n@@ -498,7 +498,7 @@ void add_exclude(const char *string, const char *base,\n {\n \tstruct exclude *x;\n \tint patternlen;\n-\tint flags;\n+\tunsigned int flags;\n \tint nowildcardlen;\n \n \tparse_exclude_pattern(&string, &patternlen, &flags, &nowildcardlen);\ndiff --git a/dir.h b/dir.h\nindex cd46f30..6d205f0 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -27,7 +27,7 @@ struct exclude {\n \tint nowildcardlen;\n \tconst char *base;\n \tint baselen;\n-\tint flags;\n+\tunsigned int flags;\t\t/* EXC_FLAG_* */\n \n \t/*\n \t * Counting starts from 1 for line numbers in ignore files,\n@@ -241,7 +241,7 @@ extern struct exclude_list *add_exclude_list(struct dir_struct *dir,\n extern int add_excludes_from_file_to_list(const char *fname, const char *base, int baselen,\n \t\t\t\t\t  struct exclude_list *el, int check_index);\n extern void add_excludes_from_file(struct dir_struct *, const char *fname);\n-extern void parse_exclude_pattern(const char **string, int *patternlen, int *flags, int *nowildcardlen);\n+extern void parse_exclude_pattern(const char **string, int *patternlen, unsigned int *flags, int *nowildcardlen);\n extern void add_exclude(const char *string, const char *base,\n \t\t\tint baselen, struct exclude_list *el, int srcpos);\n extern void clear_exclude_list(struct exclude_list *el);\n-- \n2.7.1.339.g0233b80\n"},{"id":"278792","messageId":"56CA46A3.9060404@moritzneeb.de","threadId":"41458","inReplyTo":"1456053189-5221-1-git-send-email-sauravsachidanand@gmail.com","subject":"Re: [PATCH] GSoC Micoproject: Hunt down signed int flags","fromName":"Moritz Neeb","fromEmail":"lists@moritzneeb.de","sentAt":"2016-02-21T23:22:11Z","receivedAt":"2016-02-21T23:22:11Z","isPatch":true,"sender":{"key":"lists@moritzneeb.de","avatar":null},"body":"Thanks for your patch. Just to let you know, this will be my first\nreview, but I hope it will be helpful anyway. I will mostly review your\ncommit text. First some general remarks:\n\nThe text you are submitting with you email is directly used as commit\nmessage (the email subject as well, as the first line). You might want\nto take care that the description is useful as a \"bit of history\", I\nwill give examples of what I mean below. Some guidelines for that can be\nfound in \"Documentation/SubmittingPatches\" section (2).\n\nOn 02/21/2016 12:13 PM, Saurav Sachidanand wrote:\n> This is patch is for a suggested micro project for GSoC 2016; namely,\n> that of searching for a field of a struct that is of signed integral\n> type and used as a collection of multiple bits, and converting it to\n> an unsigned type if the MSB isn’t used in any special way.\n\nEspecially, you might not want to include the fact that this is a GSoC\nproject. You might want to add this kind of information in the\n\"notes\"-section of your email after the three dashes \"---\", this will be\nskipped when the patch is applied.\n\n> Two structs, `pattern` defined in attr.c and `exclude` defined in dir.h,\n> have a `flags` field of signed int type. \n\nI have never seen the Markdown-style quotes ` in git.git commits. To be\nin the same style as previous git (which helps e.g. in readability\nbecause it is homogeneous), you can use the \"-quote for code. Or even\nleave them out if its clear from context.\n\n> The fields of both structs take\n> on values from the same set of positive integers {1, 4, 8, 16},\n> enumerated through the marco EXC_FLAG_*.\n\nmarco -> macro.\n\nI'd say this is a good observation to state.\n\nMaybe it's also helpful to further explain why the two structs are\nlogically connected, or if that turns out to be false, to split up you\nchanges into two commits. I am not fully convinced that it should be one\ncommit.\n\n>`pattern` is used only in attr.c,\n> and `exclude` is used only in builtin/check-ignore.c and dir.c, and in\n> those files, either, the value of `flags` is checked using the `&` operator\n> (e.g.: flags & EXC_FLAG_NODIR), or the value of `flags` is first set to 0\n> and then set to any one of {1, 4, 8, 16} using the `|=` operator\n> (e.g.: flags |= EXC_FLAG_NODIR). And, so it does not appear that the MSB\n> of `flags` is used in any special way.\n\nThis is the conclusion that is needed, but you might want state it more\ndirect, like \"the MSB is not used...\".\n\n> Therefore, I thought to change the\n> type of `flags` in the definitions of both structs to `unsigned int`.\n> Furthermore, `flags` is passed by reference (of `pattern` in attr.c and of\n> `exclude` in dir.c) to the function `parse_exclude_pattern` defined in\n> dir.c, that accepts an `int *` type for `flags`.\n> When make was run, it gave\n> a warning for ‘converting between pointers to integer types of different\n> sign’, so I changed the type of that respective argument to `unsigned int *`.\n\nI think this explanation can be left out or has to be replaced by\nsomething less compiler-driven, i.e. why does it actually make sense for\nparse_exclude_pattern to have an unsigned int flags as parameter.\n\n> In the end, running make to build didn’t produce any more warnings, and\n> running make in t/ didn’t produce any breakage that wasn’t ‘#TODO known\n> breakage’.\n\nFor the tests it is, from what I've seen, just assumed that you ran them\n(and the reviewers/the maintainer will confirm this for themselves), so\nno need to mention it. But it's good you ran them.\n\n> I also thought it’d be helpful to add the comment /* EXC_FLAG_* */ next\n> to `flags` of `exclude`, just like it exists for `flags` of `pattern`.\n\nI would reformulate this as well more direct to something like \"when\nwe're at it, document exclude->flags as EXC_FLAG\".\n\nThanks,\nMoritz\n"},{"id":"278808","messageId":"CAPig+cSDG8duN750fbD=RnEaLSQ6OnHc2w77LEdTXuApT97pJg@mail.gmail.com","threadId":"41458","inReplyTo":"56CA46A3.9060404@moritzneeb.de","subject":"Re: [PATCH] GSoC Micoproject: Hunt down signed int flags","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-22T00:48:39Z","receivedAt":"2016-02-22T00:48:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 21, 2016 at 6:22 PM, Moritz Neeb <lists@moritzneeb.de> wrote:\n> Thanks for your patch. Just to let you know, this will be my first\n> review, but I hope it will be helpful anyway. I will mostly review your\n> commit text. First some general remarks:\n\nThanks for taking on this review. I think you've covered everything I\nwas going to say (thus saving me a lot of time). I agree with pretty\nmuch everything you said, with (I think) only one exception (below).\n\n> The text you are submitting with you email is directly used as commit\n> message (the email subject as well, as the first line). You might want\n> to take care that the description is useful as a \"bit of history\", I\n> will give examples of what I mean below. Some guidelines for that can be\n> found in \"Documentation/SubmittingPatches\" section (2).\n>\n> On 02/21/2016 12:13 PM, Saurav Sachidanand wrote:\n>> This is patch is for a suggested micro project for GSoC 2016; namely,\n>> that of searching for a field of a struct that is of signed integral\n>> type and used as a collection of multiple bits, and converting it to\n>> an unsigned type if the MSB isn’t used in any special way.\n>\n> Especially, you might not want to include the fact that this is a GSoC\n> project. You might want to add this kind of information in the\n> \"notes\"-section of your email after the three dashes \"---\", this will be\n> skipped when the patch is applied.\n\nCorrect. That this was a GSoC mini-project is not interesting in the\npermanent project history, but it's helpful information for reviewers,\nso you'd place it in the commentary section after the \"---\" just below\nyour sign-off.\n\n>> Two structs, `pattern` defined in attr.c and `exclude` defined in dir.h,\n>> have a `flags` field of signed int type.\n>\n> I have never seen the Markdown-style quotes ` in git.git commits. To be\n> in the same style as previous git (which helps e.g. in readability\n> because it is homogeneous), you can use the \"-quote for code. Or even\n> leave them out if its clear from context.\n>\n>> The fields of both structs take\n>> on values from the same set of positive integers {1, 4, 8, 16},\n>> enumerated through the marco EXC_FLAG_*.\n>\n> marco -> macro.\n>\n> I'd say this is a good observation to state.\n>\n> Maybe it's also helpful to further explain why the two structs are\n> logically connected, or if that turns out to be false, to split up you\n> changes into two commits. I am not fully convinced that it should be one\n> commit.\n\nIf I'm reading the code correctly, I think these changes are\ninterconnected, thus they ought to remain together in a single patch.\nSpecifically, the change to the signature of parse_exclude_pattern()\nin dir.h has an impact on both structs. Thus, it should not be\nnecessary to explain further that these structs are connected (they\naren't really); rather the changes to both are merely natural\nconsequence of the change to parse_exclude_pattern()'s signature.\n\n>>`pattern` is used only in attr.c,\n>> and `exclude` is used only in builtin/check-ignore.c and dir.c, and in\n>> those files, either, the value of `flags` is checked using the `&` operator\n>> (e.g.: flags & EXC_FLAG_NODIR), or the value of `flags` is first set to 0\n>> and then set to any one of {1, 4, 8, 16} using the `|=` operator\n>> (e.g.: flags |= EXC_FLAG_NODIR). And, so it does not appear that the MSB\n>> of `flags` is used in any special way.\n>\n> This is the conclusion that is needed, but you might want state it more\n> direct, like \"the MSB is not used...\".\n>\n>> Therefore, I thought to change the\n>> type of `flags` in the definitions of both structs to `unsigned int`.\n>> Furthermore, `flags` is passed by reference (of `pattern` in attr.c and of\n>> `exclude` in dir.c) to the function `parse_exclude_pattern` defined in\n>> dir.c, that accepts an `int *` type for `flags`.\n>> When make was run, it gave\n>> a warning for ‘converting between pointers to integer types of different\n>> sign’, so I changed the type of that respective argument to `unsigned int *`.\n>\n> I think this explanation can be left out or has to be replaced by\n> something less compiler-driven, i.e. why does it actually make sense for\n> parse_exclude_pattern to have an unsigned int flags as parameter.\n\nRight, the entire paragraph in the commit message could be collapsed\nto something like:\n\n    Since the MSB of parse_exclude_pattern()'s 'flags' argument\n    is not special, change its type from 'int' to 'unsigned' to better\n    reflect this.\n\nEverything else is implied by the above. The reader understands\nimplicitly that changing the types in the structs is a natural\nconsequence of changing the signature of parse_exclude_pattern(), thus\nneed not be stated explicitly. The bit about compiler warnings and\nsuch is equally obvious and need not be mentioned.\n\nOne other comment: In this project, I think it is more common to give\nthese \"flags\" variables type \"unsigned\" rather than \"unsigned int\".\n\n>> In the end, running make to build didn’t produce any more warnings, and\n>> running make in t/ didn’t produce any breakage that wasn’t ‘#TODO known\n>> breakage’.\n>\n> For the tests it is, from what I've seen, just assumed that you ran them\n> (and the reviewers/the maintainer will confirm this for themselves), so\n> no need to mention it. But it's good you ran them.\n>\n>> I also thought it’d be helpful to add the comment /* EXC_FLAG_* */ next\n>> to `flags` of `exclude`, just like it exists for `flags` of `pattern`.\n>\n> I would reformulate this as well more direct to something like \"when\n> we're at it, document exclude->flags as EXC_FLAG\".\n\nYep.\n"},{"id":"278851","messageId":"CACsJy8CbGY2BNF=VYtJFReVTycae18bdkqRhmVQi76COvm5f0w@mail.gmail.com","threadId":"41458","inReplyTo":"1456053189-5221-1-git-send-email-sauravsachidanand@gmail.com","subject":"Re: [PATCH] GSoC Micoproject: Hunt down signed int flags","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-22T09:32:55Z","receivedAt":"2016-02-22T09:32:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Feb 21, 2016 at 6:13 PM, Saurav Sachidanand\n<sauravsachidanand@gmail.com> wrote:\n> This is patch is for a suggested micro project for GSoC 2016; namely,\n> that of searching for a field of a struct that is of signed integral\n> type and used as a collection of multiple bits, and converting it to\n> an unsigned type if the MSB isn’t used in any special way.\n\nif you use gcc, you can try to build git with -Wsign-conversion (i.e.\n\"make CFLAGS=-Wsign-conversion\") with and without your patch then see\nif there are any new warnings. I only checked dir.c and attr.c, there\nwere 4 new warnings. From a quick look, I think gcc was correct, you\njust need to convert some more \"int\" to unsigned int\" to prevent\nimplicit conversion.\n-- \nDuy\n"}]}