* Thomas Gleixner <t...@linutronix.de> wrote:
> +Variable types
> +^^^^^^^^^^^^^^
> +
> +Please use the proper u8, u16, u32, u64 types for variables which are meant
> +to describe hardware or are used as arguments for functions which access
> +hardware. These types are clearly defining the bit width and avoid
> +truncation, expansion and 32/64 bit confusion.
> +
> +u64 is also recommended in code which would become ambiguous for 32bit when
> +'unsigned long' would be used instead. While in such situations 'unsigned
> +long long' could be used as well, u64 is shorter and also clearly shows
> +that the operation is required to be 64bit wide independent of the target
> +CPU.
> +
> +Please use 'unsigned int' instead of 'unsigned'.
s/for 32bit
/for 32-bit kernels
s/64bit wide
/64 bits wide
> +Constants
> +^^^^^^^^^
> +
> +Please do not use literal (hexa)decimal numbers in code or initializers.
> +Either use proper defines which have descriptive names or consider using
> +an enum.
I believe there should be an exception for 'obvious' literal values like
0 and 1.
I.e. the above is mostly a rule that is intended to cover undocumented
'magic' numbers.
I.e. how about this wording:
+Constants
+^^^^^^^^^
+
+Please do not use magic literal (hexa)decimal numbers when interfacing
+with hardware where the number has an unclear origin in code or
+initializers. I.e. "no magic numbers".
+
+Either use proper defines which have descriptive names or use an enum.
+
+Using obvious 0/1 literal values is fine in most cases.
?
> +
> +
> +Struct declarations and initializers
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Struct declarations should align the struct member names in a tabular
> +fashion::
> +
> + struct bar_order {
> + unsigned int guest_id;
> + int ordered_item;
> + struct menu *menu;
> + };
> +
> +Please avoid documenting struct members within the declaration, because
> +this often results in strangely formatted comments and the struct members
> +become obfuscated::
> +
> + struct bar_order {
> + unsigned int guest_id; /* Unique guest id */
[ Sidenote: there's whitespace damage (extra spaces) in the text here. ]
> + int ordered_item;
> + /* Pointer to a menu instance which contains all the drinks */
> + struct menu *menu;
> + };
> +
> +Instead, please consider using the kernel-doc format in a comment preceding
> +the struct declaration, which is easier to read and has the added advantage
> +of including the information in the kernel documentation, for example, as
> +follows::
I disagree slightly here. While adding kernel-doc format is fine of
course, so are in-line comments which I frequently use.
This form is particularly helpful for more complex structures. Have a
look at 'struct fpu' for example:
/*
* Highest level per task FPU state data structure that
* contains the FPU register state plus various FPU
* state fields:
*/
struct fpu {
/*
* @last_cpu:
*
* Records the last CPU on which this context was loaded into
* FPU registers. (In the lazy-restore case we might be
* able to reuse FPU registers across multiple context switches
* this way, if no intermediate task used the FPU.)
*
* A value of -1 is used to indicate that the FPU state in context
* memory is newer than the FPU state in registers, and that the
* FPU state should be reloaded next time the task is run.
*/
unsigned int last_cpu;
/*
* @initialized:
*
* This flag indicates whether this context is initialized: if the task
* is not running then we can restore from this context, if the task
* is running then we should save into this context.
*/
unsigned char initialized;
/*
* @state:
*
* In-memory copy of all FPU registers that we save/restore
* over context switches. If the task is using the FPU then
* the registers in the FPU are more recent than this state
* copy. If the task context-switches away then they get
* saved here and represent the FPU state.
*/
union fpregs_state state;
/*
* WARNING: 'state' is dynamically-sized. Do not put
* anything after it here.
*/
};
The in-line freestanding comments is perfectly structured and readable as
well, and this is analogous to the 'freestanding comments' style for C
statements.
We also have occasional examples where tail comments are fine, such as:
/*
* The legacy x87 FPU state format, as saved by FSAVE and
* restored by the FRSTOR instructions:
*/
struct fregs_state {
u32 cwd; /* FPU Control Word */
u32 swd; /* FPU Status Word */
u32 twd; /* FPU Tag Word */
u32 fip; /* FPU IP Offset */
u32 fcs; /* FPU IP Selector */
u32 foo; /* FPU Operand Pointer Offset */
u32 fos; /* FPU Operand Pointer Selector */
/* 8*10 bytes for each FP-reg = 80 bytes: */
u32 st_space[20];
/* Software status information [not touched by FSAVE]: */
u32 status;
};
But I'd not complicate the style guide with that.
> +Static struct initializers must use C99 initializers and should also be
> +aligned in a tabular fashion::
> +
> + static struct foo statfoo = {
> + .a = 0,
> + .plain_integer = CONSTANT_DEFINE_OR_ENUM,
> + .bar = &statbar,
> + };
> +
Yeah, and maybe also add a note about the final comma:
+ Note that while C99 syntax allows the omission of the final comma, we
+ recommend the use of a comma on the last line because it makes
+ reordering and addition of new lines easier, and makes such future
+ patches slightly easier to read as well.
?
Thanks,
Ingo
