[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 <address@hidden> wrote:
> Add C type rules from libvirt HACKING. Also include
> a description of special QEMU scalar types.
>
> Signed-off-by: Blue Swirl <address@hidden>
> ---
>  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.