On Thu, Mar 24, 2016 at 3:56 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Thu, Mar 24, 2016 at 4:01 PM, work <motroniii@xxxxxxxxx> wrote:
>> On 03/24/2016 10:51 PM, Stefan Beller wrote:
>>> On Thu, Mar 24, 2016 at 12:41 PM, Motroni Igor <motroniii@xxxxxxxxx>
>>> wrote:
>>>> From: Pontifik <motroniii@xxxxxxxxx>
>>>
>>> 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
This is what I meant. (I did not give my grep terms as reasoning as they were
not as exactly as yours. Redoing the search using your more exact terms
however makes me wonder if my advice was the right thing.
% git grep -E 'unsigned\s+int\s+flags' ./*.h | wc -l
16
% git grep -E 'unsigned\s+flags' ./*.h | wc -l
17
So there is no pattern that the version without int is more common.
Maybe my exposure to the code was accidentally in a way such that
I ever only saw the version without int.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html