Re: [PATCH 1/2] Modified flag field type in rev_list_info struct in bisect.h. There is no need for flag field to be signed, as it is not supposed to be used as decimal.

> On Thu, Mar 24, 2016 at 4:01 PM, work <motroniii@gmail.com> wrote:
>> On 03/24/2016 10:51 PM, Stefan Beller wrote:
>>> On Thu, Mar 24, 2016 at 12:41 PM, Motroni Igor <motroniii@gmail.com>
>>> wrote:
>>>> From: Pontifik <motroniii@gmail.com>
>>>
>>> Here is a good place to put reasoning for why this is a good idea.
>>> I see you have a long subject, so maybe we can shorten the first line
>>> (down to less than ~ 80 characters) and put the longer explanation
>>> here.
>>>
>>> How about:
>>>
>>>    bisect: use unsigned for flag field
>>>
>>>    The flags are usually used as a unsigned variable, because it makes
>>>    bit operations easier to follow.
>>
>> Yep, it's definitely a good idea to shorten subject in order to put more
>> explanations in body of a message.
>
> It is also very important is to explain that you audited all clients
> of this field and found that none of them treat the sign bit specially
> (for instance, by checking the value with '< 0' or some such),
> therefore such a change is safe, in addition to making the code
> clearer.
>
> As an example, a diligent reviewer may wonder why you changed this
> field to 'unsigned' but not the 'flags' variable in
> rev-list.c:show_bisect_vars() to which this field is assigned. This is
> something your re-roll of the patch should take into consideration.
>
>>>> diff --git a/bisect.h b/bisect.h
>>>> index acd12ef..a979a7f 100644
>>>> --- a/bisect.h
>>>> +++ b/bisect.h
>>>> @@ -16,7 +16,7 @@ extern struct commit_list *filter_skipped(struct
>>>> commit_list *list,
>>>>
>>>>   struct rev_list_info {
>>>>          struct rev_info *revs;
>>>> -       int flags;
>>>> +       unsigned int flags;
>>>
>>> You can also drop the int here and make it just
>>> unsigned.
>>
>> In fact, I just wanted to keep this code clear to understand (as I see
>> this). Unsigned short is also unsigned, but a reader should know that
>> "unsigned" type stands for "unsigned int". Anyway, I'll keep this in mind in
>> future, thanks a lot.
>
> While it's true that 'unsigned short' is indeed never negative, the
> clueful reader should never accidentally interpret bare 'unsigned' as
> anything other than the native word size. However, I think what Stefan
> really meant is that, in this code base, it is (somewhat) more common
> to declare these "flags" variables as 'unsigned' rather than 'unsigned
> int':
>
>     % git grep -E 'unsigned\s+flags' | wc -l
>     80
>     % git grep -E 'unsigned\s+int\s+flags' | wc -l
>     57