[PATCH][GSoc] Changed signed flags to unsigned type

Vedant Bassi· Mar 9, 2017, 04:24 UTC · lore

As part of my microproject :

Use unsigned integral type for collection of bits: Pick one field of a structure that (1) is of signed integral type and (2) is used as a collection of multiple bits. Discuss if there is a good reason why it has to be a signed integral field and change it to an unsigned type otherwise.

More ref: https://public-inbox.org/git/xmqqsiebrlez.fsf@gitster.dls.corp.google.com http://stackoverflow.com/questions/29795170/usage-of-signed-vs-unsigned-variables-for-flags-in-c

I have found several structures where a signed int was used on flags for bitwise & to check various cases.

Show changes to 3 files +11 −2

bisect.h, builtin/add.c, parse-options.h

diff --git a/bisect.h b/bisect.h

index a979a7f..4b562a8 100644

--- a/bisect.h

+++ b/bisect.h

@@ -16,6 +16,8 @@ extern struct commit_list *filter_skipped(struct
commit_list *list,



 struct rev_list_info {

        struct rev_info *revs;

+

+ // int flags changed to unsigned int

        unsigned int flags;

        int show_timestamp;

        int hdr_termination;

diff --git a/builtin/add.c b/builtin/add.c

index 9f53f02..1212eea 100644

--- a/builtin/add.c

+++ b/builtin/add.c

@@ -26,6 +26,8 @@ static int patch_interactive, add_interactive,
edit_interactive;

 static int take_worktree_changes;



 struct update_callback_data {

+

+ // flags supposed to be unsigned

        int flags;

        int add_errors;

 };

diff --git a/parse-options.h b/parse-options.h

index dcd8a09..ad180c9 100644

--- a/parse-options.h

+++ b/parse-options.h

@@ -107,7 +107,9 @@ struct option {

        const char *argh;

        const char *help;



-   int flags;

+ // int flags changed to unsigned

+

+ unsigned int flags;

        parse_opt_cb *callback;

        intptr_t defval;

 };

@@ -201,7 +203,10 @@ struct parse_opt_ctx_t {

        const char **out;

        int argc, cpidx, total;

        const char *opt;

-   int flags;

+

+ // int flags changed to unsigned

+

+ unsigned int flags;

        const char *prefix;

 };



-------

result : the changes were made in bisect.h , parse-options.h and  builtin/add.c

I have not yet  tested these changes.

Christian Couder· Mar 9, 2017, 07:49 UTC · re: Vedant Bassi · lore

Re: [PATCH][GSoc] Changed signed flags to unsigned type

On Thu, Mar 9, 2017 at 5:24 AM, Vedant Bassi <sharababy.dev@gmail.com> wrote:

Show 13 quoted lines

> As part of my microproject :
>
> Use unsigned integral type for collection of bits:
> Pick one field of a structure that (1) is of signed integral type and
> (2) is used as a collection of multiple bits. Discuss if there is a
> good reason why it has to be a signed integral field and change it to
> an unsigned type otherwise.
>
> More ref: https://public-inbox.org/git/xmqqsiebrlez.fsf@gitster.dls.corp.google.com
> http://stackoverflow.com/questions/29795170/usage-of-signed-vs-unsigned-variables-for-flags-in-c
>
> I have found several structures where a signed int was used on flags
> for bitwise & to check various cases.

This email has a title that starts with "[PATCH]" but it doesn't contain a real patch that can be applied using `git am` for example. Please look at https://git.github.io/SoC-2017-Microprojects/ and at Documentation/SubmittingPatches about how patches should be created.

Show 20 quoted lines

> diff --git a/bisect.h b/bisect.h
>
> index a979a7f..4b562a8 100644
>
> --- a/bisect.h
>
> +++ b/bisect.h
>
> @@ -16,6 +16,8 @@ extern struct commit_list *filter_skipped(struct
> commit_list *list,
>
>
>
>  struct rev_list_info {
>
>         struct rev_info *revs;
>
> +
>
> + // int flags changed to unsigned int

You don't need to add such comments as "what has been done" will be obvious when looking at the commit. It could be interesting to explain "why the change is made" in the commit message though.

If there is something subtle in the code or something that could save a reader some time if it was documented, then a comment might be useful, but anyway comments should use "/* ... */" markers, not "//".

Show 5 quoted lines

>         unsigned int flags;
>
>         int show_timestamp;
>
>         int hdr_termination;

[...]

> result : the changes were made in bisect.h , parse-options.h and  builtin/add.c
>
> I have not yet  tested these changes.

I think sending just one patch for bisect.h is ok. If you really want you could send another patch for parse-options.h and yet another one for builtin/add.c, all in the same patch series.

Anyway please test that the patches can be applied (using git am) and that they look good (compared with other commits) when applied before sending them to the list. And yeah it is also a good idea to also check that the test suite still passes after each patch before sending them.

patch[GSoc] Changed signed flags to unsigned type