Hello, I don't want to make waves, but I do care about certain aspects of code quality, so... I propose to allow (encourage, even) the use of the standard C99 type, bool, in libvirt, but not in public interfaces[1]. There are already uses in cgroup.c and xmlrpc.c, as well as those in the gnulib "c-ctype.h" header which is included from many of libvirt's .c files. The motivation is to make the code as readable as possible. When you see the declaration of an "int" variable, member, or function, that type gives you no clue whether it is used as a boolean. If you see a few uses that treat it as boolean, that's still no guarantee, and it may be non-trivial to ensure that there isn't some non-boolean value with a special meaning. However, if you see a "bool" variable, you do have that guarantee. This is one reason to using the tighter, more descriptive type. In addition, if ever you need to go back to using "int", it's trivial to automate the substitution, e.g., -Dint=bool or sed s/int/bool/. On the other hand, it is very hard to automatically identify "int" variables that may safely be converted to "bool". Portability is no problem whatsoever, thanks to gnulib's stdbool module, which ensures that there is a working <stdbool.h> header in the compiler's include path. libvirt even includes/runs a test to ensure that the <stdbool.h> in use (from gnulib or from the system) works properly. Of course, if you agree on principle, we should update HACKING to reflect the policy, so I've taken a shot at that. But if we start talking about using the right type for boolean variables, we can't avoid the more general question of scalars, signedness, etc. And once you mention scalars, you have to mention pointers and const-correctness, so it got longer still. Patch below. Jim [1]: We can use "bool" reliably in libvirt-internal code, due to the fact that it is guaranteed to be compiled with a usable <stdbool.h> header. We cannot make such a guarantee for all applications that compile code including libvirt's public headers, <libvirt.h> and <virterror.h>. P.S. I confess I have a vested interested. I wrote a patch recently that declares a local to be of type "bool", and would like to ensure that policy permits doing that. From: Jim Meyering <meyering(a)redhat.com&gt; Date: Fri, 12 Dec 2008 09:45:31 +0100 Subject: [PATCH] HACKING: use the right type --- HACKING | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 46 insertions(+), 0 deletions(-) diff --git a/HACKING b/HACKING index 3945833..511c8cc 100644 --- a/HACKING +++ b/HACKING @@ -91,6 +91,52 @@ Usually they're in macro definitions or strings, and should be converted anyhow. +C types +======= +Use the right type. + +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). +If a variable has boolean semantics, give it the "bool" type +and use the corresponding "true" and "false" macros. It's ok +to include <stdbool.h>, since libvirt's use of gnulib ensures +that it exists and is usable. +In the unusual event that you require a specific width, use a +standard type like int32_t, uint32_t, uint64_t, etc. + +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 int or ssize_t, +use types for your own variables that match. + +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. + +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. + + Low level memory management =========================== -- 1.6.1.rc2.299.gead4c.dirty