[Qemu-devel] [PATCH 2/5] CODING_STYLE: add C type rules

[Qemu-devel] [PATCH 2/5] CODING_STYLE: add C type rules

[Blue Swirl]

Add C type rules from libvirt HACKING. Also include
a description of special QEMU scalar types.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 CODING_STYLE |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/CODING_STYLE b/CODING_STYLE
index c4c09ab..3f10d72 100644
--- a/CODING_STYLE
+++ b/CODING_STYLE
@@ -92,3 +92,59 @@ indentation to track nesting:
 #if defined(HAVE_POSIX_FALLOCATE) && !defined(HAVE_FALLOCATE)
 # define fallocate(a,ignored,b,c) posix_fallocate(a,b,c)
 #endif
+
+6. C types
+
+Use the right type.
+
+6.1. Scalars
+
+If you're using "int" or "long", odds are good that there's a better type.
+If a variable is counting something, be sure to declare it with an
+unsigned type.
+If it's memory-size-related, use size_t (use ssize_t only if required).
+If it's file-size related, use uintmax_t, or maybe off_t.
+If it's file-offset related (i.e., signed), use off_t.
+If it's just counting small numbers use "unsigned int";
+(on all but oddball embedded systems, you can assume that that
+type is at least four bytes wide).
+
+In the event that you require a specific width, use a standard type like
+int32_t, uint32_t, uint64_t, etc.  The specific types are mandatory for
+vmstate.
+
+Don't use Linux kernel internal types like u32, __u32 or __le32.
+
+Use target_phys_addr_t for hardware physical addresses except pcibus_t
+for PCI addresses.  Use target_ulong (or abi_ulong) for CPU
+virtual addresses, however devices should not need to use target_ulong.
+
+While using "bool" is good for readability, it comes with minor caveats:
+ - Don't use "bool" in places where the type size must be constant across
+   all systems, like public interfaces and on-the-wire protocols.
+ - Don't compare a bool variable against the literal, "true",
+   since a value with a logical non-false value need not be "1".
+   I.e., don't write "if (seen == true) ...".  Rather, write "if (seen)...".
+
+Of course, take all of the above with a grain of salt.  If you're about
+to use some system interface that requires a type like size_t, pid_t or
+off_t, use matching types for any corresponding variables.
+
+Also, if you try to use e.g., "unsigned int" as a type, and that
+conflicts with the signedness of a related variable, sometimes
+it's best just to use the *wrong* type, if "pulling the thread"
+and fixing all related variables would be too invasive.
+
+Finally, while using descriptive types is important, be careful not to
+go overboard.  If whatever you're doing causes warnings, or requires
+casts, then reconsider or ask for help.
+
+6.2. Pointers
+
+Ensure that all of your pointers are "const-correct".
+Unless a pointer is used to modify the pointed-to storage,
+give it the "const" attribute.  That way, the reader knows
+up-front that this is a read-only pointer.  Perhaps more
+importantly, if we're diligent about this, when you see a non-const
+pointer, you're guaranteed that it is used to modify the storage
+it points to, or it is aliased to another pointer that is.
-- 
1.6.2.4

[Qemu-devel] Re: [PATCH 2/5] CODING_STYLE: add C type rules

[Blue Swirl]

On Thu, Aug 12, 2010 at 5:50 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> Add C type rules from libvirt HACKING. Also include
> a description of special QEMU scalar types.
>
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> ---
>  CODING_STYLE |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 56 insertions(+), 0 deletions(-)
>
> diff --git a/CODING_STYLE b/CODING_STYLE
> index c4c09ab..3f10d72 100644
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -92,3 +92,59 @@ indentation to track nesting:
>  #if defined(HAVE_POSIX_FALLOCATE) && !defined(HAVE_FALLOCATE)
>  # define fallocate(a,ignored,b,c) posix_fallocate(a,b,c)
>  #endif
> +
> +6. C types
> +
> +Use the right type.
> +
> +6.1. Scalars
> +
> +If you're using "int" or "long", odds are good that there's a better type.
> +If a variable is counting something, be sure to declare it with an
> +unsigned type.
> +If it's memory-size-related, use size_t (use ssize_t only if required).
> +If it's file-size related, use uintmax_t, or maybe off_t.
> +If it's file-offset related (i.e., signed), use off_t.
> +If it's just counting small numbers use "unsigned int";
> +(on all but oddball embedded systems, you can assume that that
> +type is at least four bytes wide).
> +
> +In the event that you require a specific width, use a standard type like
> +int32_t, uint32_t, uint64_t, etc.  The specific types are mandatory for
> +vmstate.
> +
> +Don't use Linux kernel internal types like u32, __u32 or __le32.
> +
> +Use target_phys_addr_t for hardware physical addresses except pcibus_t
> +for PCI addresses.  Use target_ulong (or abi_ulong) for CPU
> +virtual addresses, however devices should not need to use target_ulong.
> +
> +While using "bool" is good for readability, it comes with minor caveats:
> + - Don't use "bool" in places where the type size must be constant across
> +   all systems, like public interfaces and on-the-wire protocols.
> + - Don't compare a bool variable against the literal, "true",
> +   since a value with a logical non-false value need not be "1".
> +   I.e., don't write "if (seen == true) ...".  Rather, write "if (seen)...".
> +
> +Of course, take all of the above with a grain of salt.  If you're about
> +to use some system interface that requires a type like size_t, pid_t or
> +off_t, use matching types for any corresponding variables.
> +
> +Also, if you try to use e.g., "unsigned int" as a type, and that
> +conflicts with the signedness of a related variable, sometimes
> +it's best just to use the *wrong* type, if "pulling the thread"
> +and fixing all related variables would be too invasive.
> +
> +Finally, while using descriptive types is important, be careful not to
> +go overboard.  If whatever you're doing causes warnings, or requires
> +casts, then reconsider or ask for help.
> +
> +6.2. Pointers
> +
> +Ensure that all of your pointers are "const-correct".
> +Unless a pointer is used to modify the pointed-to storage,
> +give it the "const" attribute.  That way, the reader knows
> +up-front that this is a read-only pointer.  Perhaps more
> +importantly, if we're diligent about this, when you see a non-const
> +pointer, you're guaranteed that it is used to modify the storage
> +it points to, or it is aliased to another pointer that is.

We don't use uintmax_t, so that part should be dropped.

Original libvirt text mentioned some problems with <stdbool.h> and
described their solution. Perhaps we should use the same solution or
deprecate <stdbool.h>?

There's some overlap between 6 and 3. Typedef part from 3 should be moved here.

The whole piece should be reworded as a guideline (as opposed to a
strict rule). Most of this is common sense and in some cases there are
multiple equally good choices that can be selected. Only the vmstate
and address part should be strict rules.

Re: [Qemu-devel] [PATCH 2/5] CODING_STYLE: add C type rules

[Jes Sorensen]

On 08/12/10 19:50, Blue Swirl wrote:
> +While using "bool" is good for readability, it comes with minor caveats:
> + - Don't use "bool" in places where the type size must be constant across
> +   all systems, like public interfaces and on-the-wire protocols.
> + - Don't compare a bool variable against the literal, "true",
> +   since a value with a logical non-false value need not be "1".
> +   I.e., don't write "if (seen == true) ...".  Rather, write "if (seen)...".

I'd strongly discourage the use of bool in any code. It doesn't provide
anything guarantees and you are not sure about the size of it. Using int
is safer. IMHO bool is one of the worse examples of changes to the C
standard :(

bool foo = false;
foo++;
if (foo == true)....

The other thing that might be worth mentioning in the int/long section
is that long is complicated in broken development environments such as
Windows where it is only 32 bit :(

Cheers,
Jes

Re: [Qemu-devel] [PATCH 2/5] CODING_STYLE: add C type rules

[Blue Swirl]

On Tue, Aug 17, 2010 at 8:09 AM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
> On 08/12/10 19:50, Blue Swirl wrote:
>> +While using "bool" is good for readability, it comes with minor caveats:
>> + - Don't use "bool" in places where the type size must be constant across
>> +   all systems, like public interfaces and on-the-wire protocols.
>> + - Don't compare a bool variable against the literal, "true",
>> +   since a value with a logical non-false value need not be "1".
>> +   I.e., don't write "if (seen == true) ...".  Rather, write "if (seen)...".
>
> I'd strongly discourage the use of bool in any code. It doesn't provide
> anything guarantees and you are not sure about the size of it. Using int
> is safer. IMHO bool is one of the worse examples of changes to the C
> standard :(
>
> bool foo = false;
> foo++;
> if (foo == true)....

It's already in use. Could you write a replacement rule which forbids
any new bool use and welcomes patches to remove any existing cases?

> The other thing that might be worth mentioning in the int/long section
> is that long is complicated in broken development environments such as
> Windows where it is only 32 bit :(

Only on Win64 (http://technet.microsoft.com/en-us/library/bb496995.aspx)
but we don't support that.

We could add something like traditional "don't mix int and pointer
sizes" and not require intptr_t in place of longs.

Re: [Qemu-devel] [PATCH 2/5] CODING_STYLE: add C type rules

[Richard Henderson]

On 08/17/2010 01:09 AM, Jes Sorensen wrote:
> On 08/12/10 19:50, Blue Swirl wrote:
>> +While using "bool" is good for readability, it comes with minor caveats:
>> + - Don't use "bool" in places where the type size must be constant across
>> +   all systems, like public interfaces and on-the-wire protocols.
>> + - Don't compare a bool variable against the literal, "true",
>> +   since a value with a logical non-false value need not be "1".
>> +   I.e., don't write "if (seen == true) ...".  Rather, write "if (seen)...".
> 
> I'd strongly discourage the use of bool in any code.

I strongly disagree.  The use of "bool", even if you ignore stdbool.h
and do "typedef int bool", is valuable documentation in the code.


r~

Re: [Qemu-devel] [PATCH 2/5] CODING_STYLE: add C type rules

[malc]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1788 bytes --]

On Tue, 17 Aug 2010, Blue Swirl wrote:

> On Tue, Aug 17, 2010 at 8:09 AM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
> > On 08/12/10 19:50, Blue Swirl wrote:
> >> +While using "bool" is good for readability, it comes with minor caveats:
> >> + - Don't use "bool" in places where the type size must be constant across
> >> +   all systems, like public interfaces and on-the-wire protocols.
> >> + - Don't compare a bool variable against the literal, "true",
> >> +   since a value with a logical non-false value need not be "1".
> >> +   I.e., don't write "if (seen == true) ...".  Rather, write "if (seen)...".
> >
> > I'd strongly discourage the use of bool in any code. It doesn't provide
> > anything guarantees and you are not sure about the size of it. Using int
> > is safer. IMHO bool is one of the worse examples of changes to the C
> > standard :(
> >
> > bool foo = false;
> > foo++;
> > if (foo == true)....
> 
> It's already in use. Could you write a replacement rule which forbids
> any new bool use and welcomes patches to remove any existing cases?
> 
> > The other thing that might be worth mentioning in the int/long section
> > is that long is complicated in broken development environments such as
> > Windows where it is only 32 bit :(

There's absolutely nothing broken about LLP64 it's as valid as any other
ABI. (That's to Jes)

> 
> Only on Win64 (http://technet.microsoft.com/en-us/library/bb496995.aspx)
> but we don't support that.

We "don't support" that only because nobody has merged Filip Navara's
port[1]

> We could add something like traditional "don't mix int and pointer
> sizes" and not require intptr_t in place of longs.
> 

[1] http://repo.or.cz/w/qemu/navara.git/commit/e777e89c4be2b80b37043e72fe6158da5ea4bf6c

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 2/5] CODING_STYLE: add C type rules

[Jes Sorensen]

On 08/17/10 20:39, Richard Henderson wrote:
> On 08/17/2010 01:09 AM, Jes Sorensen wrote:
>> On 08/12/10 19:50, Blue Swirl wrote:
>>> +While using "bool" is good for readability, it comes with minor caveats:
>>> + - Don't use "bool" in places where the type size must be constant across
>>> +   all systems, like public interfaces and on-the-wire protocols.
>>> + - Don't compare a bool variable against the literal, "true",
>>> +   since a value with a logical non-false value need not be "1".
>>> +   I.e., don't write "if (seen == true) ...".  Rather, write "if (seen)...".
>>
>> I'd strongly discourage the use of bool in any code.
> 
> I strongly disagree.  The use of "bool", even if you ignore stdbool.h
> and do "typedef int bool", is valuable documentation in the code.

I guess we have to agree to disagree then. IMHO it just masks the real
type and you end up with cases where people pass it back and forth randomly.

Cheers,
Jes

Re: [Qemu-devel] [PATCH 2/5] CODING_STYLE: add C type rules

[Jes Sorensen]

On 08/17/10 20:55, malc wrote:
> On Tue, 17 Aug 2010, Blue Swirl wrote:
>>> The other thing that might be worth mentioning in the int/long section
>>> is that long is complicated in broken development environments such as
>>> Windows where it is only 32 bit :(
> 
> There's absolutely nothing broken about LLP64 it's as valid as any other
> ABI. (That's to Jes)

Well it works if you program for it, but it still doesn't make it any
good when you can't keep a pointer in a long to apply arithmetic to it.
Anyway point with the documentation is to make it clear that we rely on
being able to do long foo = (long)ptr;

Cheers,
Jes

Re: [Qemu-devel] [PATCH 2/5] CODING_STYLE: add C type rules

[malc]

On Tue, 17 Aug 2010, Jes Sorensen wrote:

> On 08/17/10 20:55, malc wrote:
> > On Tue, 17 Aug 2010, Blue Swirl wrote:
> >>> The other thing that might be worth mentioning in the int/long section
> >>> is that long is complicated in broken development environments such as
> >>> Windows where it is only 32 bit :(
> > 
> > There's absolutely nothing broken about LLP64 it's as valid as any other
> > ABI. (That's to Jes)
> 
> Well it works if you program for it, but it still doesn't make it any
> good when you can't keep a pointer in a long to apply arithmetic to it.
> Anyway point with the documentation is to make it clear that we rely on
> being able to do long foo = (long)ptr;

Which isn't (and never was) sanctioned by any standard, IOW not good.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 2/5] CODING_STYLE: add C type rules

[Jes Sorensen]

On 08/17/10 21:24, malc wrote:
> On Tue, 17 Aug 2010, Jes Sorensen wrote:
> 
>> On 08/17/10 20:55, malc wrote:
>>> On Tue, 17 Aug 2010, Blue Swirl wrote:
>>>>> The other thing that might be worth mentioning in the int/long section
>>>>> is that long is complicated in broken development environments such as
>>>>> Windows where it is only 32 bit :(
>>>
>>> There's absolutely nothing broken about LLP64 it's as valid as any other
>>> ABI. (That's to Jes)
>>
>> Well it works if you program for it, but it still doesn't make it any
>> good when you can't keep a pointer in a long to apply arithmetic to it.
>> Anyway point with the documentation is to make it clear that we rely on
>> being able to do long foo = (long)ptr;
> 
> Which isn't (and never was) sanctioned by any standard, IOW not good.

Well maybe this is where the problem is. Not being able to do this means
that we need a special integer type to cover this case if we wanted to
work on win64. Switching to long long would generate bad code on 32 bit
archs so thats not an option.

Depending on your viewpoint it is either it not being a standard that is
bad, or the LLP64 that is bad.

Anyway this is personal preference.

Jes

Re: [Qemu-devel] [PATCH 2/5] CODING_STYLE: add C type rules

[Anthony Liguori]

On 08/17/2010 02:43 PM, Jes Sorensen wrote:
> On 08/17/10 21:24, malc wrote:
>    
>> On Tue, 17 Aug 2010, Jes Sorensen wrote:
>>
>>      
>>> On 08/17/10 20:55, malc wrote:
>>>        
>>>> On Tue, 17 Aug 2010, Blue Swirl wrote:
>>>>          
>>>>>> The other thing that might be worth mentioning in the int/long section
>>>>>> is that long is complicated in broken development environments such as
>>>>>> Windows where it is only 32 bit :(
>>>>>>              
>>>> There's absolutely nothing broken about LLP64 it's as valid as any other
>>>> ABI. (That's to Jes)
>>>>          
>>> Well it works if you program for it, but it still doesn't make it any
>>> good when you can't keep a pointer in a long to apply arithmetic to it.
>>> Anyway point with the documentation is to make it clear that we rely on
>>> being able to do long foo = (long)ptr;
>>>        
>> Which isn't (and never was) sanctioned by any standard, IOW not good.
>>      
> Well maybe this is where the problem is. Not being able to do this means
> that we need a special integer type to cover this case if we wanted to
> work on win64. Switching to long long would generate bad code on 32 bit
> archs so thats not an option.
>    

FWIW, that type is intptr_t and it was introduced in C99.

Regards,

Anthony Liguori

> Depending on your viewpoint it is either it not being a standard that is
> bad, or the LLP64 that is bad.
>
> Anyway this is personal preference.
>
> Jes
>
>
>    

Re: [Qemu-devel] [PATCH 2/5] CODING_STYLE: add C type rules

[malc]

On Tue, 17 Aug 2010, Jes Sorensen wrote:

> On 08/17/10 21:24, malc wrote:
> > On Tue, 17 Aug 2010, Jes Sorensen wrote:
> > 
> >> On 08/17/10 20:55, malc wrote:
> >>> On Tue, 17 Aug 2010, Blue Swirl wrote:
> >>>>> The other thing that might be worth mentioning in the int/long section
> >>>>> is that long is complicated in broken development environments such as
> >>>>> Windows where it is only 32 bit :(
> >>>
> >>> There's absolutely nothing broken about LLP64 it's as valid as any other
> >>> ABI. (That's to Jes)
> >>
> >> Well it works if you program for it, but it still doesn't make it any
> >> good when you can't keep a pointer in a long to apply arithmetic to it.
> >> Anyway point with the documentation is to make it clear that we rely on
> >> being able to do long foo = (long)ptr;
> > 
> > Which isn't (and never was) sanctioned by any standard, IOW not good.
> 
> Well maybe this is where the problem is. Not being able to do this means
> that we need a special integer type to cover this case if we wanted to
> work on win64. Switching to long long would generate bad code on 32 bit
> archs so thats not an option.

That's why [u]intptr_t was invented.

> 
> Depending on your viewpoint it is either it not being a standard that is
> bad, or the LLP64 that is bad.

This doesn't really parse for me.

> 
> Anyway this is personal preference.
> 
> Jes
> 

-- 
mailto:av1474@comtv.ru

[Qemu-devel] Re: [PATCH 2/5] CODING_STYLE: add C type rules

[Paolo Bonzini]

On 08/17/2010 08:39 PM, Richard Henderson wrote:
>>  On 08/12/10 19:50, Blue Swirl wrote:
>>>  +While using "bool" is good for readability, it comes with minor caveats:
>>>  + - Don't use "bool" in places where the type size must be constant across
>>>  +   all systems, like public interfaces and on-the-wire protocols.
>>>  + - Don't compare a bool variable against the literal, "true",
>>>  +   since a value with a logical non-false value need not be "1".
>>>  +   I.e., don't write "if (seen == true) ...".  Rather, write "if (seen)...".
>>
>>  I'd strongly discourage the use of bool in any code.
>
> I strongly disagree.  The use of "bool", even if you ignore stdbool.h
> and do "typedef int bool", is valuable documentation in the code.

I think "bool" is fine, but it should be either stdbool.h or a typedef. 
  Using stdbool.h only when it is present is going to introduce bugs the 
day someone relies on the magic properties of the C99 bool.

Paolo

[Qemu-devel] Re: [PATCH 2/5] CODING_STYLE: add C type rules

[Jes Sorensen]

On 08/18/10 10:35, Paolo Bonzini wrote:
> On 08/17/2010 08:39 PM, Richard Henderson wrote:
>>>  I'd strongly discourage the use of bool in any code.
>>
>> I strongly disagree.  The use of "bool", even if you ignore stdbool.h
>> and do "typedef int bool", is valuable documentation in the code.
> 
> I think "bool" is fine, but it should be either stdbool.h or a typedef.
>  Using stdbool.h only when it is present is going to introduce bugs the
> day someone relies on the magic properties of the C99 bool.

This is exactly the problem, it doesn't buy us anything, except for
#ifdef chaos. We're better off sticking to int, then we know what we
have and avoiding this mess.

Jes

Re: [Qemu-devel] Re: [PATCH 2/5] CODING_STYLE: add C type rules

[Kevin Wolf]

Am 18.08.2010 10:35, schrieb Paolo Bonzini:
> On 08/17/2010 08:39 PM, Richard Henderson wrote:
>>>  On 08/12/10 19:50, Blue Swirl wrote:
>>>>  +While using "bool" is good for readability, it comes with minor caveats:
>>>>  + - Don't use "bool" in places where the type size must be constant across
>>>>  +   all systems, like public interfaces and on-the-wire protocols.
>>>>  + - Don't compare a bool variable against the literal, "true",
>>>>  +   since a value with a logical non-false value need not be "1".
>>>>  +   I.e., don't write "if (seen == true) ...".  Rather, write "if (seen)...".
>>>
>>>  I'd strongly discourage the use of bool in any code.
>>
>> I strongly disagree.  The use of "bool", even if you ignore stdbool.h
>> and do "typedef int bool", is valuable documentation in the code.
> 
> I think "bool" is fine, but it should be either stdbool.h or a typedef. 
>   Using stdbool.h only when it is present is going to introduce bugs the 
> day someone relies on the magic properties of the C99 bool.

We rely on C99 anyway, so stdbool.h should always be present (and in
fact, it is used unconditionally today).

Kevin

Re: [Qemu-devel] Re: [PATCH 2/5] CODING_STYLE: add C type rules

[Paolo Bonzini]

On 08/18/2010 12:30 PM, Kevin Wolf wrote:
> Am 18.08.2010 10:35, schrieb Paolo Bonzini:
>> On 08/17/2010 08:39 PM, Richard Henderson wrote:
>>>>   On 08/12/10 19:50, Blue Swirl wrote:
>>>>>   +While using "bool" is good for readability, it comes with minor caveats:
>>>>>   + - Don't use "bool" in places where the type size must be constant across
>>>>>   +   all systems, like public interfaces and on-the-wire protocols.
>>>>>   + - Don't compare a bool variable against the literal, "true",
>>>>>   +   since a value with a logical non-false value need not be "1".
>>>>>   +   I.e., don't write "if (seen == true) ...".  Rather, write "if (seen)...".
>>>>
>>>>   I'd strongly discourage the use of bool in any code.
>>>
>>> I strongly disagree.  The use of "bool", even if you ignore stdbool.h
>>> and do "typedef int bool", is valuable documentation in the code.
>>
>> I think "bool" is fine, but it should be either stdbool.h or a typedef.
>>    Using stdbool.h only when it is present is going to introduce bugs the
>> day someone relies on the magic properties of the C99 bool.
>
> We rely on C99 anyway, so stdbool.h should always be present (and in
> fact, it is used unconditionally today).

Right.  However, this is wrong then:

- Don't compare a bool variable against the literal, "true",
   since a value with a logical non-false value need not be "1".
   I.e., don't write "if (seen == true) ...".  Rather, write "if
   (seen)...".

I mean, I'm all for using "if (seen)" but bool *does* normalize logical 
non-false values to "1".  I'd remove the second line (making it just a 
coding style issue) and add something like this:

- Do not rely on the fact that bool normalizes logical non-false values
   to 1.  So, write "x = true" instead of "x++" and "x = !x" instead of
   "x--".
- Similarly, when x is a bool, it may be clearer to avoid
   "x |= y".  Instead, use either "x = x || y" (if short circuiting
   is acceptable or even desirable) or "x |= (y != 0)".

Probably a bit too verbose, but you get the idea.

Paolo

Re: [Qemu-devel] [PATCH 2/5] CODING_STYLE: add C type rules

[Avi Kivity]

  On 08/17/2010 11:09 AM, Jes Sorensen wrote:
> On 08/12/10 19:50, Blue Swirl wrote:
>> +While using "bool" is good for readability, it comes with minor caveats:
>> + - Don't use "bool" in places where the type size must be constant across
>> +   all systems, like public interfaces and on-the-wire protocols.
>> + - Don't compare a bool variable against the literal, "true",
>> +   since a value with a logical non-false value need not be "1".
>> +   I.e., don't write "if (seen == true) ...".  Rather, write "if (seen)...".
> I'd strongly discourage the use of bool in any code. It doesn't provide
> anything guarantees

It's actually better than int:

      int flag = word & BIT;

can lose the BIT if it is outside the range of an int, whereas

     bool flag = word & BIT;

will be true if BIT is set.  Not to mention the improved readability 
(which appears to be a downside to C fans).

> and you are not sure about the size of it. Using int
> is safer. IMHO bool is one of the worse examples of changes to the C
> standard :(
>
> bool foo = false;
> foo++;

What's incrementing a bool supposed to mean?  Even more true?

> if (foo == true)....

As it happens, this will succeed.

> The other thing that might be worth mentioning in the int/long section
> is that long is complicated in broken development environments such as
> Windows where it is only 32 bit :(

It's only complicated if you assume sizeof(long) == sizeof(void *).  
While we're used to it from Linux, all you need is s/long/intptr_t/ and 
you're all set.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 2/5] CODING_STYLE: add C type rules

[Avi Kivity]

  On 08/17/2010 10:15 PM, Jes Sorensen wrote:
>
>> I strongly disagree.  The use of "bool", even if you ignore stdbool.h
>> and do "typedef int bool", is valuable documentation in the code.
> I guess we have to agree to disagree then. IMHO it just masks the real
> type and you end up with cases where people pass it back and forth randomly.

In C99, bool is a real type.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] Re: [PATCH 2/5] CODING_STYLE: add C type rules

[Avi Kivity]

  On 08/18/2010 04:57 PM, Paolo Bonzini wrote:
> On 08/18/2010 12:30 PM, Kevin Wolf wrote:
>> Am 18.08.2010 10:35, schrieb Paolo Bonzini:
>>> On 08/17/2010 08:39 PM, Richard Henderson wrote:
>>>>>   On 08/12/10 19:50, Blue Swirl wrote:
>>>>>>   +While using "bool" is good for readability, it comes with 
>>>>>> minor caveats:
>>>>>>   + - Don't use "bool" in places where the type size must be 
>>>>>> constant across
>>>>>>   +   all systems, like public interfaces and on-the-wire protocols.
>>>>>>   + - Don't compare a bool variable against the literal, "true",
>>>>>>   +   since a value with a logical non-false value need not be "1".
>>>>>>   +   I.e., don't write "if (seen == true) ...".  Rather, write 
>>>>>> "if (seen)...".
>>>>>
>>>>>   I'd strongly discourage the use of bool in any code.
>>>>
>>>> I strongly disagree.  The use of "bool", even if you ignore stdbool.h
>>>> and do "typedef int bool", is valuable documentation in the code.
>>>
>>> I think "bool" is fine, but it should be either stdbool.h or a typedef.
>>>    Using stdbool.h only when it is present is going to introduce 
>>> bugs the
>>> day someone relies on the magic properties of the C99 bool.
>>
>> We rely on C99 anyway, so stdbool.h should always be present (and in
>> fact, it is used unconditionally today).
>
> Right.  However, this is wrong then:
>
> - Don't compare a bool variable against the literal, "true",
>   since a value with a logical non-false value need not be "1".
>   I.e., don't write "if (seen == true) ...".  Rather, write "if
>   (seen)...".
>
> I mean, I'm all for using "if (seen)" but bool *does* normalize 
> logical non-false values to "1".  I'd remove the second line (making 
> it just a coding style issue) and add something like this:
>
> - Do not rely on the fact that bool normalizes logical non-false values
>   to 1.  So, write "x = true" instead of "x++" and "x = !x" instead of
>   "x--".
> - Similarly, when x is a bool, it may be clearer to avoid
>   "x |= y".  Instead, use either "x = x || y" (if short circuiting
>   is acceptable or even desirable) or "x |= (y != 0)".
>
> Probably a bit too verbose, but you get the idea.

_Bool can have values of 0 or 1, so all of the above should actually work.

_Bool convert_to_bool(long long x)
{
     return x;
}


    0:   48 85 ff                test   %rdi,%rdi
    3:   0f 95 c0                setne  %al
    6:   c3                      retq

void incr(_Bool* b)
{
     ++*b;
}

   10:   c6 07 01                movb   $0x1,(%rdi)
   13:   c3                      retq


If _Bool were int, the first example would fail.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] Re: [PATCH 2/5] CODING_STYLE: add C type rules

[Paolo Bonzini]

On 08/18/2010 06:55 PM, Avi Kivity wrote:
>> - Do not rely on the fact that bool normalizes logical non-false values
>> to 1. So, write "x = true" instead of "x++" and "x = !x" instead of
>> "x--".
>> - Similarly, when x is a bool, it may be clearer to avoid
>> "x |= y". Instead, use either "x = x || y" (if short circuiting
>> is acceptable or even desirable) or "x |= (y != 0)".
>>
>> Probably a bit too verbose, but you get the idea.
>
> _Bool can have values of 0 or 1, so all of the above should actually work.

Yes, I'd like to rule those out even though they work, because I find 
them confusing.  The compiler is smart enough to generate the same code 
in both cases.

Paolo

Re: [Qemu-devel] [PATCH 2/5] CODING_STYLE: add C type rules

[Jes Sorensen]

On 08/18/10 18:46, Avi Kivity wrote:
>  On 08/17/2010 10:15 PM, Jes Sorensen wrote:
>>
>>> I strongly disagree.  The use of "bool", even if you ignore stdbool.h
>>> and do "typedef int bool", is valuable documentation in the code.
>> I guess we have to agree to disagree then. IMHO it just masks the real
>> type and you end up with cases where people pass it back and forth
>> randomly.
> 
> In C99, bool is a real type.

Kinda real, I would qualify it more as a pseudo type. It doesn't map to
any register size or even instruction actions. Most processors, at least
the ones I have programmed, tend to treat it as zero == false,
everything else == true. For structure packing it's ugly.

Jes

Re: [Qemu-devel] [PATCH 2/5] CODING_STYLE: add C type rules

[Avi Kivity]

  On 08/19/2010 10:58 AM, Jes Sorensen wrote:
>
>> In C99, bool is a real type.
> Kinda real, I would qualify it more as a pseudo type. It doesn't map to
> any register size or even instruction actions.

Neither do int, short, long, long long, or signed short long float.

> Most processors, at least
> the ones I have programmed, tend to treat it as zero == false,
> everything else == true.

Processors don't have _Bool.

> For structure packing it's ugly.

Use uint*_t for externally visible structures.  use _Bool for internal 
booleans.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] Re: [PATCH 2/5] CODING_STYLE: add C type rules

[Avi Kivity]

  On 08/19/2010 10:51 AM, Paolo Bonzini wrote:
>> _Bool can have values of 0 or 1, so all of the above should actually 
>> work.
>
>
> Yes, I'd like to rule those out even though they work, because I find 
> them confusing.  The compiler is smart enough to generate the same 
> code in both cases.

One problem with coding styles is that people think they replace common 
sense.  They don't, you shouldn't write ++x when you aren't counting 
even if there is no explicit rule in coding style forbidding it.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 2/5] CODING_STYLE: add C type rules

[Jes Sorensen]

On 08/19/10 10:10, Avi Kivity wrote:
>  On 08/19/2010 10:58 AM, Jes Sorensen wrote:
>>
>>> In C99, bool is a real type.
>> Kinda real, I would qualify it more as a pseudo type. It doesn't map to
>> any register size or even instruction actions.
> 
> Neither do int, short, long, long long, or signed short long float.

Well most processors support 32 bit register ops, even when running in
64 bit mode. long is the maximum register size in pretty much any sane
setup, which is why it is such a mess that M$ took the easy way out when
they picked for win64. Tons of user code will have portability problems
because of this.

>> Most processors, at least
>> the ones I have programmed, tend to treat it as zero == false,
>> everything else == true.
> 
> Processors don't have _Bool.

No but something has to generate code to handle it.

>> For structure packing it's ugly.
> 
> Use uint*_t for externally visible structures.  use _Bool for internal
> booleans.

Structure packing is more than passing structures to external processes
or saving them, it's also cache line alignment. In your case we should
change pretty much any data structure in QEMU that is in a critical data
path to use *int*_t.

Jes

Re: [Qemu-devel] [PATCH 2/5] CODING_STYLE: add C type rules

[Avi Kivity]

  On 08/19/2010 11:17 AM, Jes Sorensen wrote:
> On 08/19/10 10:10, Avi Kivity wrote:
>>   On 08/19/2010 10:58 AM, Jes Sorensen wrote:
>>>> In C99, bool is a real type.
>>> Kinda real, I would qualify it more as a pseudo type. It doesn't map to
>>> any register size or even instruction actions.
>> Neither do int, short, long, long long, or signed short long float.
> Well most processors support 32 bit register ops, even when running in
> 64 bit mode.

8-bit processors have 8-bit register size and 16-bit ints.  Some 
processors have 36-bit ints.  If you only code for processors which have 
Linux ports, and for those ports, then everything will be the same.  
Surprise!

> long is the maximum register size in pretty much any sane
> setup,

Not on those 8-bit machines.

> which is why it is such a mess that M$ took the easy way out when
> they picked for win64. Tons of user code will have portability problems
> because of this.

They did that to avoid portability problems, presumably.

>>> Most processors, at least
>>> the ones I have programmed, tend to treat it as zero == false,
>>> everything else == true.
>> Processors don't have _Bool.
> No but something has to generate code to handle it.

gcc

>>> For structure packing it's ugly.
>> Use uint*_t for externally visible structures.  use _Bool for internal
>> booleans.
> Structure packing is more than passing structures to external processes
> or saving them, it's also cache line alignment. In your case we should
> change pretty much any data structure in QEMU that is in a critical data
> path to use *int*_t.

Not at all, _Bool is smaller or equal to int and thus can only improve 
packing.

If you want to align explicitly, use alignment attributes.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/5] CODING_STYLE: add C type rules

[malc]

On Thu, 19 Aug 2010, Avi Kivity wrote:

>  On 08/19/2010 11:17 AM, Jes Sorensen wrote:
> > On 08/19/10 10:10, Avi Kivity wrote:
> > >   On 08/19/2010 10:58 AM, Jes Sorensen wrote:
> > > > > In C99, bool is a real type.
> > > > Kinda real, I would qualify it more as a pseudo type. It doesn't map to
> > > > any register size or even instruction actions.
> > > Neither do int, short, long, long long, or signed short long float.
> > Well most processors support 32 bit register ops, even when running in
> > 64 bit mode.
> 
> 8-bit processors have 8-bit register size and 16-bit ints.  Some processors
> have 36-bit ints.  If you only code for processors which have Linux ports, and
> for those ports, then everything will be the same.  Surprise!
> 
> > long is the maximum register size in pretty much any sane
> > setup,
> 
> Not on those 8-bit machines.
> 
> > which is why it is such a mess that M$ took the easy way out when
> > they picked for win64. Tons of user code will have portability problems
> > because of this.
> 
> They did that to avoid portability problems, presumably.

http://blogs.msdn.com/b/oldnewthing/archive/2005/01/31/363790.aspx

> 
> > > > Most processors, at least
> > > > the ones I have programmed, tend to treat it as zero == false,
> > > > everything else == true.
> > > Processors don't have _Bool.
> > No but something has to generate code to handle it.
> 
> gcc
> 
> > > > For structure packing it's ugly.
> > > Use uint*_t for externally visible structures.  use _Bool for internal
> > > booleans.
> > Structure packing is more than passing structures to external processes
> > or saving them, it's also cache line alignment. In your case we should
> > change pretty much any data structure in QEMU that is in a critical data
> > path to use *int*_t.
> 
> Not at all, _Bool is smaller or equal to int and thus can only improve
> packing.

That's not what standard says:

6.2.5#2

An object declared as type _Bool is large enough to store the values 0 and 

It later goes on to say (6.7.2.1 fn120):

While the number of bits in a _Bool object is at least CHAR_BIT, the width
(number of sign and value bits) of a _Bool may be just 1 bit.

> 
> If you want to align explicitly, use alignment attributes.
> 
> 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 2/5] CODING_STYLE: add C type rules

[Avi Kivity]

  On 08/19/2010 03:52 PM, malc wrote:
>
>> Not at all, _Bool is smaller or equal to int and thus can only improve
>> packing.
> That's not what standard says:
>
> 6.2.5#2
>
> An object declared as type _Bool is large enough to store the values 0 and
>
> It later goes on to say (6.7.2.1 fn120):
>
> While the number of bits in a _Bool object is at least CHAR_BIT, the width
> (number of sign and value bits) of a _Bool may be just 1 bit.
>

That's not in conflict (well, the standard allows sizeof(_Bool) > 
sizeof(int), but that's unlikely).


-- 
error compiling committee.c: too many arguments to function