{"thread":{"id":"45327","subject":"[PATCH][GSoc] Changed signed flags to unsigned type","startedAt":"2017-03-09T04:25:17Z","lastAt":"2017-03-09T07:49:49Z","messageCount":2,"participants":["Vedant Bassi","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"313612","messageId":"CACczA6V6t4f6TTT=CJfqsuCtbYuM1QNh8AgtOwqRt7pz4VMeRA@mail.gmail.com","threadId":"45327","inReplyTo":null,"subject":"[PATCH][GSoc] Changed signed flags to unsigned type","fromName":"Vedant Bassi","fromEmail":"sharababy.dev@gmail.com","sentAt":"2017-03-09T04:24:27Z","receivedAt":"2017-03-09T04:25:17Z","isPatch":true,"sender":{"key":"sharababy.dev@gmail.com","avatar":null},"body":"As part of my microproject :\n\nUse unsigned integral type for collection of bits:\nPick one field of a structure that (1) is of signed integral type and\n(2) is used as a collection of multiple bits. Discuss if there is a\ngood reason why it has to be a signed integral field and change it to\nan unsigned type otherwise.\n\nMore ref: https://public-inbox.org/git/xmqqsiebrlez.fsf@gitster.dls.corp.google.com\nhttp://stackoverflow.com/questions/29795170/usage-of-signed-vs-unsigned-variables-for-flags-in-c\n\nI have found several structures where a signed int was used on flags\nfor bitwise & to check various cases.\n\n\ndiff --git a/bisect.h b/bisect.h\n\nindex a979a7f..4b562a8 100644\n\n--- a/bisect.h\n\n+++ b/bisect.h\n\n@@ -16,6 +16,8 @@ extern struct commit_list *filter_skipped(struct\ncommit_list *list,\n\n\n\n struct rev_list_info {\n\n        struct rev_info *revs;\n\n+\n\n+ // int flags changed to unsigned int\n\n        unsigned int flags;\n\n        int show_timestamp;\n\n        int hdr_termination;\n\ndiff --git a/builtin/add.c b/builtin/add.c\n\nindex 9f53f02..1212eea 100644\n\n--- a/builtin/add.c\n\n+++ b/builtin/add.c\n\n@@ -26,6 +26,8 @@ static int patch_interactive, add_interactive,\nedit_interactive;\n\n static int take_worktree_changes;\n\n\n\n struct update_callback_data {\n\n+\n\n+ // flags supposed to be unsigned\n\n        int flags;\n\n        int add_errors;\n\n };\n\ndiff --git a/parse-options.h b/parse-options.h\n\nindex dcd8a09..ad180c9 100644\n\n--- a/parse-options.h\n\n+++ b/parse-options.h\n\n@@ -107,7 +107,9 @@ struct option {\n\n        const char *argh;\n\n        const char *help;\n\n\n\n-   int flags;\n\n+ // int flags changed to unsigned\n\n+\n\n+ unsigned int flags;\n\n        parse_opt_cb *callback;\n\n        intptr_t defval;\n\n };\n\n@@ -201,7 +203,10 @@ struct parse_opt_ctx_t {\n\n        const char **out;\n\n        int argc, cpidx, total;\n\n        const char *opt;\n\n-   int flags;\n\n+\n\n+ // int flags changed to unsigned\n\n+\n\n+ unsigned int flags;\n\n        const char *prefix;\n\n };\n\n\n\n-------\n\nresult : the changes were made in bisect.h , parse-options.h and  builtin/add.c\n\nI have not yet  tested these changes.\n"},{"id":"313617","messageId":"CAP8UFD1k=uDRnrGJRw=NG9NmRVd8yMXcE_jyB=dpeKOf75HbCw@mail.gmail.com","threadId":"45327","inReplyTo":"CACczA6V6t4f6TTT=CJfqsuCtbYuM1QNh8AgtOwqRt7pz4VMeRA@mail.gmail.com","subject":"Re: [PATCH][GSoc] Changed signed flags to unsigned type","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-03-09T07:49:25Z","receivedAt":"2017-03-09T07:49:49Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Mar 9, 2017 at 5:24 AM, Vedant Bassi <sharababy.dev@gmail.com> wrote:\n> As part of my microproject :\n>\n> Use unsigned integral type for collection of bits:\n> Pick one field of a structure that (1) is of signed integral type and\n> (2) is used as a collection of multiple bits. Discuss if there is a\n> good reason why it has to be a signed integral field and change it to\n> an unsigned type otherwise.\n>\n> More ref: https://public-inbox.org/git/xmqqsiebrlez.fsf@gitster.dls.corp.google.com\n> http://stackoverflow.com/questions/29795170/usage-of-signed-vs-unsigned-variables-for-flags-in-c\n>\n> I have found several structures where a signed int was used on flags\n> for bitwise & to check various cases.\n\nThis email has a title that starts with \"[PATCH]\" but it doesn't\ncontain a real patch that can be applied using `git am` for example.\nPlease look at https://git.github.io/SoC-2017-Microprojects/ and at\nDocumentation/SubmittingPatches about how patches should be created.\n\n> diff --git a/bisect.h b/bisect.h\n>\n> index a979a7f..4b562a8 100644\n>\n> --- a/bisect.h\n>\n> +++ b/bisect.h\n>\n> @@ -16,6 +16,8 @@ extern struct commit_list *filter_skipped(struct\n> commit_list *list,\n>\n>\n>\n>  struct rev_list_info {\n>\n>         struct rev_info *revs;\n>\n> +\n>\n> + // int flags changed to unsigned int\n\nYou don't need to add such comments as \"what has been done\" will be\nobvious when looking at the commit. It could be interesting to explain\n\"why the change is made\" in the commit message though.\n\nIf there is something subtle in the code or something that could save\na reader some time if it was documented, then a comment might be\nuseful, but anyway comments should use \"/* ... */\" markers, not \"//\".\n\n>         unsigned int flags;\n>\n>         int show_timestamp;\n>\n>         int hdr_termination;\n\n[...]\n\n> result : the changes were made in bisect.h , parse-options.h and  builtin/add.c\n>\n> I have not yet  tested these changes.\n\nI think sending just one patch for bisect.h is ok. If you really want\nyou could send another patch for parse-options.h and yet another one\nfor builtin/add.c, all in the same patch series.\n\nAnyway please test that the patches can be applied (using git am) and\nthat they look good (compared with other commits) when applied before\nsending them to the list. And yeah it is also a good idea to also\ncheck that the test suite still passes after each patch before sending\nthem.\n"}]}