Message-ID: <20181108080406.GF20032@gmail.com>
Date:   Thu, 8 Nov 2018 09:04:06 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>, x86@...nel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Paul McKenney <paulmck@...ux.vnet.ibm.com>,
        John Stultz <john.stultz@...aro.org>,
        Arnaldo Carvalho de Melo <acme@...hat.com>,
        Frederic Weisbecker <frederic@...nel.org>,
        Jonathan Corbet <corbet@....net>,
        Andy Lutomirski <luto@...nel.org>,
        Marc Zyngier <marc.zyngier@....com>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Will Deacon <will.deacon@....com>,
        Mark Brown <broonie@...nel.org>,
        Dan Williams <dan.j.williams@...el.com>
Subject: Re: [patch 2/2] Documentation/process: Add tip tree handbook


* Thomas Gleixner <tglx@...utronix.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

Powered by blists - more mailing lists