Re: [Xen-devel] [PATCH RFC] CODING_STYLE: document intended usage of types

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 02/21/2018 03:55 PM, Andrew Cooper wrote:
> On 19/02/18 13:30, Jan Beulich wrote:
>>>>> On 19.02.18 at 14:12, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 19/02/18 08:44, Jan Beulich wrote:
>>>> --- a/CODING_STYLE
>>>> +++ b/CODING_STYLE
>>>> @@ -88,6 +88,26 @@ Braces should be omitted for blocks with
>>>>  if ( condition )
>>>>      single_statement();
>>>>  
>>>> +Types
>>>> +-----
>>>> +
>>>> +Use basic C types and C standard mandated typedef-s where possible (and
>>>> +with preference in this order).  This in particular means to avoid u8,
>>>> +u16, etc despite those types continuing to exist in our code base.
>>>> +Fixed width types should only be used when a fixed width quantity is
>>>> +meant (which for example may be a value read from or to be written to a
>>>> +register).
>>>> +
>>>> +When signedness matters, qualify plain char, short, int, long, and
>>>> +long long with "signed" or "unsigned".  Signedness is specifically
>>>> +considered to matter when the valid value range of a variable covers
>>>> +only non-negative values.  The prime example of such is a variable used
>>>> +to index an array (negative array indexes, while they may occur, are
>>>> +rather rare).
>>> As is evident from the other threads on the subject, I am very
>>> definitely -1 for littering our codebase with signed in cases like this.
>> Some context for those not having followed the earlier discussion:
>> There being quite a number of cases in the code base where plain
>> int or long are used when no negative values are ever expected
>> (or even possible) to be held by the respective variables, I would
>> prefer if we made explicit when signedness of a variable matters.
>> This then also eliminates signedness concerns for plain char and bit
>> fields (for both of these one already needs to explicitly add "signed"
>> when negative values are intended to be held by the variable/field,
>> at least if we don't want to tie ourselves to compiler specific
>> behavior).
>>
>>> IMO they do nothing but harm readibility.
>> How does making something explicit harm readability?
> 
> Using 'signed' when it is unnecessary strictly adds to code volume.
> 
> Expecting the use of signed in contexts where it is not necessary is
> going to add an extra round trip to every review.  Even if the core
> developers get used to using it, casual submitters are not going to use
> it, because it is very a-typical.  This constitutes a net reduction in
> review bandwidth.

This second bit is the biggest one for me: Adding this change will take
a significant amount of effort, and cause a significant amount of
annoyance to contributors, as well as increase the review workload (by
having to review an otherwise perfectly good patch a second time).

I also agree that it doesn't add anything (using a non-unsigned version
already implies 'I think this should be signed'), and I also agree that
we're almost certainly not going to achieve consistency; so in my
estimation the extra cost is clearly not worth it.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel