Re: [PATCH] GSoC Micoproject: Hunt down signed int flags

On Sun, Feb 21, 2016 at 6:22 PM, Moritz Neeb [off-list ref] wrote:

Thanks for your patch. Just to let you know, this will be my first
review, but I hope it will be helpful anyway. I will mostly review your
commit text. First some general remarks:

Thanks for taking on this review. I think you've covered everything I
was going to say (thus saving me a lot of time). I agree with pretty
much everything you said, with (I think) only one exception (below).

The text you are submitting with you email is directly used as commit
message (the email subject as well, as the first line). You might want
to take care that the description is useful as a "bit of history", I
will give examples of what I mean below. Some guidelines for that can be
found in "Documentation/SubmittingPatches" section (2).

On 02/21/2016 12:13 PM, Saurav Sachidanand wrote:

quoted

This is patch is for a suggested micro project for GSoC 2016; namely,
that of searching for a field of a struct that is of signed integral
type and used as a collection of multiple bits, and converting it to
an unsigned type if the MSB isn’t used in any special way.

Especially, you might not want to include the fact that this is a GSoC
project. You might want to add this kind of information in the
"notes"-section of your email after the three dashes "---", this will be
skipped when the patch is applied.

Correct. That this was a GSoC mini-project is not interesting in the
permanent project history, but it's helpful information for reviewers,
so you'd place it in the commentary section after the "---" just below
your sign-off.

quoted

Two structs, `pattern` defined in attr.c and `exclude` defined in dir.h,
have a `flags` field of signed int type.

I have never seen the Markdown-style quotes ` in git.git commits. To be
in the same style as previous git (which helps e.g. in readability
because it is homogeneous), you can use the "-quote for code. Or even
leave them out if its clear from context.

quoted

The fields of both structs take
on values from the same set of positive integers {1, 4, 8, 16},
enumerated through the marco EXC_FLAG_*.

marco -> macro.

I'd say this is a good observation to state.

Maybe it's also helpful to further explain why the two structs are
logically connected, or if that turns out to be false, to split up you
changes into two commits. I am not fully convinced that it should be one
commit.

If I'm reading the code correctly, I think these changes are
interconnected, thus they ought to remain together in a single patch.
Specifically, the change to the signature of parse_exclude_pattern()
in dir.h has an impact on both structs. Thus, it should not be
necessary to explain further that these structs are connected (they
aren't really); rather the changes to both are merely natural
consequence of the change to parse_exclude_pattern()'s signature.

quoted

`pattern` is used only in attr.c,
and `exclude` is used only in builtin/check-ignore.c and dir.c, and in
those files, either, the value of `flags` is checked using the `&` operator
(e.g.: flags & EXC_FLAG_NODIR), or the value of `flags` is first set to 0
and then set to any one of {1, 4, 8, 16} using the `|=` operator
(e.g.: flags |= EXC_FLAG_NODIR). And, so it does not appear that the MSB
of `flags` is used in any special way.

This is the conclusion that is needed, but you might want state it more
direct, like "the MSB is not used...".

quoted

Therefore, I thought to change the
type of `flags` in the definitions of both structs to `unsigned int`.
Furthermore, `flags` is passed by reference (of `pattern` in attr.c and of
`exclude` in dir.c) to the function `parse_exclude_pattern` defined in
dir.c, that accepts an `int *` type for `flags`.
When make was run, it gave
a warning for ‘converting between pointers to integer types of different
sign’, so I changed the type of that respective argument to `unsigned int *`.

I think this explanation can be left out or has to be replaced by
something less compiler-driven, i.e. why does it actually make sense for
parse_exclude_pattern to have an unsigned int flags as parameter.

Right, the entire paragraph in the commit message could be collapsed
to something like:

    Since the MSB of parse_exclude_pattern()'s 'flags' argument
    is not special, change its type from 'int' to 'unsigned' to better
    reflect this.

Everything else is implied by the above. The reader understands
implicitly that changing the types in the structs is a natural
consequence of changing the signature of parse_exclude_pattern(), thus
need not be stated explicitly. The bit about compiler warnings and
such is equally obvious and need not be mentioned.

One other comment: In this project, I think it is more common to give
these "flags" variables type "unsigned" rather than "unsigned int".

quoted

In the end, running make to build didn’t produce any more warnings, and
running make in t/ didn’t produce any breakage that wasn’t ‘#TODO known
breakage’.

For the tests it is, from what I've seen, just assumed that you ran them
(and the reviewers/the maintainer will confirm this for themselves), so
no need to mention it. But it's good you ran them.

quoted

I also thought it’d be helpful to add the comment /* EXC_FLAG_* */ next
to `flags` of `exclude`, just like it exists for `flags` of `pattern`.

I would reformulate this as well more direct to something like "when
we're at it, document exclude->flags as EXC_FLAG".

Yep.