Re: [patch 2/2] Documentation/process: Add tip tree handbook

Ingo Molnar
2018-11-08T08:04:14Z

* 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