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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

We are never ever going to get consistent use of signed even under your
plan, due to the code we inherit from other projects.  This at best will
leave its use inconsistent across the codebase, and therefore add to the
development confusion we already have WRT indentation style etc.

As Coverity has demonstrated repeatedly, there is often a difference
between what people write, and what they intend to write (and this
includes yourself and myself).  Seeing a patch with an explicit "signed"
doesn't aid review, because the code still needs just as much second
guessing.  The only thing that matters is whether the reviewer can work
out if the code is correct, and that comes not from the written type,
but the context in which it is used.


I agree that submitted code should be using unsigned integers by
default, because is it is usually the correct behaviour, and the
resulting codegen is typically better.

However, I don't agree with your argument that explicit use of signed
for integers which are intended to have negative values will make
writing or reviewing code easier.  Quite the converse;  I'm confident
that it would be worse in several regards for the project as a whole.

~Andrew

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