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

From: Christian Couder <christian.couder@gmail.com>
Date: Mar 9, 2017, 07:49 UTC
Message-ID: <CAP8UFD1k=uDRnrGJRw=NG9NmRVd8yMXcE_jyB=dpeKOf75HbCw@mail.gmail.com>
In-Reply-To: <CACczA6V6t4f6TTT=CJfqsuCtbYuM1QNh8AgtOwqRt7pz4VMeRA@mail.gmail.com>

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.