Messages in this thread | | | Date | Thu, 8 Nov 2018 09:04:06 +0100 | From | Ingo Molnar <> | Subject | Re: [patch 2/2] Documentation/process: Add tip tree handbook |
| |
* Thomas Gleixner <tglx@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
| |