The following is a series of patches related to coding standards.  The first

patch updates the CodingStandards document with respect to printk debugging

and un/likely use; the second patch updates the usage string for

checkpatch.pl; the third patch introduces a new small perl script that can

be used to check coding-standards compliance on multiple source files.  This

new script, called check-coding-standards.pl, produces output in a standard

compiler-error format, which can be parsed by assorted tools including text

editors and help users locate the file that had the error and the offending

line-number more quickly.
Erez Zadok (3):

      CodingStyle updates

      Update usage string for checkpatch.pl

      New script to check coding-style compliance on multiple regular files
 Documentation/CodingStyle         |   88 +++++++++++++++++++++++++++++++++++++-

 scripts/check-coding-standards.pl |   59 +++++++++++++++++++++++++

 scripts/checkpatch.pl             |    1

 3 files changed, 146 insertions(+), 2 deletions(-)
---

Erez Zadok

ezk@cs.sunysb.edu

-

I'm not very happy with this.
"CodingStyle" should be about the big issues, not about details. Yes,

we've messed that up over the years, but let's not continue that.
In other words, I'd suggest *removing* lines from CodingStyle, not adding

them. The file has already gone from a "good general principles" to "lots

of stupid details". Let's not make it worse.
			Linus

-

I think there needs to be a "sense of fairness" attached here - CodingStyle

should cover all the stuff maintainers/reviewers are allowed to whinge about.

Otherwise, we get maintainers whinging about undocumented style rules, and

we get code submitters who are unhappy because they have no real way to tell

if their code really *is* stylistically lacking, or if they just have a

jerk maintainer who wants to keep their code out-of-tree for political

reasons.

... and no matter how many rules you put down, it's still possible to

write a code that will be awful stylistically while adhering to all of

them.  Religiously.
IOW, it's not going to eliminate that kind of fights.

-

I've run into those sort of programmers.  Unfortunately, there's no real

cure for them short of a baseball/cricket bat or similar implement.

No.
People whine too much as is. Don't give them *license* to do so.
		Linus

-

I kind of meant the *maintainers* couldn't whine about things not in CodingStyle ;)

And yet people want to give license for maintainers to whine about

parenthesis in printk's?
Wow.
							- Ted
-

No, I understood you.
I'm personally of the opinion that the automated style checking, and

having detailed rules is always a mistake. 
It's *much* better to teach people the big things. And the small things

will vary from person to person _anyway_, so it's not like you can really

document them.
		Linus

-

There's a lot of good value in having all those details, as it helps people

standardize on more common practices, including details.  I think removing

all those details may only increase the amount-of less-accepted code to be

posted, resulting in more lkml people having to repeat themselves on what

not to do.  Only now, they won't be able to point people to the CodingStyle

file for what they should do right.
Would you prefer if CodingStyle was reorganized or even split into (1)

general principles and (2) details?  Perhaps we need a CodingStylePrinciples

and a CodingStyleDetails?
IOW, where should that very useful info live?
Erez.

-

I'm certainly ok with the split into two files.
What I'm not ok with is really important stuff (indentation), and then

mixing in silly rules ("parenthesis are bad in printk's"?)
		Linus

-

OK, looking at CodingStyle, I see two kinds of chapters.  The first is stuff

that's more generic to C, and the other is more specific to Linux and how

one codes in the linux kernel.  So I propose the following:
1. we create a new file called CodingSuggestions
2. we keep in CodingStyle the following chapters
   Chapter 1: Indentation

   Chapter 2: Breaking long lines and strings

   Chapter 3: Placing Braces and Spaces

   Chapter 4: Naming

   Chapter 5: Typedefs

   Chapter 6: Functions

   Chapter 7: Centralized exiting of functions

   Chapter 8: Commenting

   Chapter 9: You've made a mess of it
   Note: I'd suggest we rename the title of ch9 to "Custom Editor

   Programming/Indentation Modes" or something more descriptive.
   Chapter 10: Kconfig configuration files

   Chapter 11: Data structures

   Chapter 12: Macros, Enums and RTL

   Chapter 15: The inline disease

   Chapter 16: Function return values and names

   Chapter 18: Editor modelines and other cruft
3. move the following chapters to CodingSuggestions:
   Chapter 13: Printing kernel messages

   Note: ch13 is the one which mentions the don't put parentheses around %d.
   Chapter 14: Allocating memory

   Chapter 17: Don't re-invent the kernel macros

   Chapter 19: branch prediction optimizations (the un/likely debacle)
4. We go through checkpatch.pl and ensure that every test in checkpatch is

   documented either in CodingStyle or in CodingSuggestions, determined by

   whether checkpatch considers it an ERROR, WARNING, or just CHECK.
I think the above chapter split will be a reasonable start, and of course we

can tweak things over time.  The general idea is that CodingStyle will

checkpatch.
But, there's something really nice abuot having to point people to just one

file instead of two.  We could also keep it all in one file, but split it

into two parts:
   Part 1: Mandatory Coding Style

   Chapter 1: indentation

   ...
   Part 2: Coding Style Suggestions

   Chapter n: printing kernel mess...
[ message continues ]

Super.  And then, in the spirit of Linus's request,

we can submit another patch that simply removes that second file

completely from the kernel source tree!
Yay!  The suits actually lose for once!

(okay, so I can fantasize if I want to).

-

Yep, you can.
The other losers are new contributors who get rejected for not

understanding implicit kernel coding styles and people who

get to try to explain to them on a repeating basis (if even

that happens).
or the kernel maintainers if coding style doesn't matter so much

and uglier code begins to be merged.
---

~Randy

-

here's a radical idea -- what about splitting the content across two

documents?  you know ... perhaps "coding style aesthetics" versus

"coding distinctions" or something like that.
oh, wait ...
rday

--

========================================================================

Robert P. J. Day

Linux Consulting, Training and Annoying Kernel Pedantry

Waterloo, Ontario, CANADA
http://crashcourse.ca

========================================================================

-

It'd be nice to split the current CodingStyle into two documents:
	- A shorter CodingStyle that gives the spirit of the style

	  (short functions, minimal nesting, logic as straightforward as

	  possible, etc.), and addresses the most commonly repeated

	  mistakes, without so much detail that people's eyes glaze

	  over.  You want to be able to recommend it to your students

	  (or whoever) in reasonable confidence that they'll actually

	  read it and have fun (leave the jokes in!).  Currently I'm

	  suspicious that it's becoming something that everybody

	  recommends but noone bothers to sit down and read anymore

	  unless they're working on it.
	- A CodingStyleReference that's just a long dry list of rules,

	  organized to make it easy to look up an individual rule when

	  needed.  That'd also take the pressure of CodingStyle to

	  accept every new detail.
It'd be a start just to revert CodingStyle to its original content and

move the rest to CodingStyleReference.  But someone would want to skim

through the CodingStyle history for any legimate corrections that we

want to keep.
--b.

-

How about CodingStyleSuggestions?  And let's make it clear they are

suggestions, and not things that checkpatch should be flaming about

unless people request the all of the annoying associated false

positives via --spam-me-harder.  :-)
       	      	      	       			  - Ted

-

FWIW, the checkpatch script is woefully out of sync with CodingStyle.  I
Erez.

-

---

~Randy

Phaedrus says that Quality is about caring.

-

1. Updates chapter 13 (printing kernel messages) to expand on the use of

   pr_debug()/pr_info(), what to avoid, and how to hook your debug code with

   kernel.h.
2. New chapter 19, branch prediction optimizations, discusses the whole

   un/likely issue.
Cc: "Kok, Auke" <auke-jan.h.kok@intel.com>

Cc: Kyle Moffett <mrmacman_g4@mac.com>

Cc: Jan Engelhardt <jengelh@computergmbh.de>

Cc: Adrian Bunk <bunk@kernel.org>

Cc: roel <12o3l@tiscali.nl>
Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>

---

 Documentation/CodingStyle |   88 +++++++++++++++++++++++++++++++++++++++++++-

 1 files changed, 86 insertions(+), 2 deletions(-)
diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle

index 7f1730f..00b29e4 100644

--- a/Documentation/CodingStyle

+++ b/Documentation/CodingStyle

@@ -643,8 +643,26 @@ Printing numbers in parentheses (%d) adds no value and should be avoided.

 There are a number of driver model diagnostic macros in <linux/device.h>

 which you should use to make sure messages are matched to the right device

 and driver, and are tagged with the right level:  dev_err(), dev_warn(),

-dev_info(), and so forth.  For messages that aren't associated with a

-particular device, <linux/kernel.h> defines pr_debug() and pr_info().

+dev_info(), and so forth.

+

+A number of people often like to define their own debugging printf's,

+wrapping printk's in #ifdef's that get turned on only when subsystem

+debugging is compiled in (e.g., dprintk, Dprintk, DPRINTK, etc.).  Please

+don't reinvent the wheel but use existing mechanisms.  For messages that

+aren't associated with a particular device, <linux/kernel.h> defines

+pr_debug() and pr_info(); the latter two translate to printk(KERN_DEBUG) and

+printk(KERN_INFO), respectively.  However, to get pr_debug() to actually

+emit the message, you'll need to turn on DEBUG in your code, which can be

+done as follows in your subsystem Makefile:

+

+ifeq ($(CONFIG_WHATEVER_DEBUG),y)

+EXTR...
[ message continues ]

Alternatively, that can be done in your source file, but doing this

in the Makefile makes good sense if you have more than one source file.

At any rate, it is done in some source files, and when it's done in

source files, #define-ing DEBUG should be done before #include
---

~Randy

Phaedrus says that Quality is about caring.

-

The abvoe hurst my eyes each time I see it.

Lately I have considered extending the kbuild syntax a bit.
Introducing

ccflags-y

asflags-y
[with same functionality as the EXTRA_CFLAGS, EXTRA_AFLAGS]

would allow us to do:
ccflags-$(CONFIG_WHATEVER_DEBUG) := -DDEBUG
Much nicer IMHO.
	Sam

-

Hi Sam,
Please do!
That is very useful for testing and developing new modules.

I learnt a lot from you in this regard and used that kind

of syntax to the extreme in some other non-kernel project

of mine. There it included also ccflags, asflags and so on.
I further split that into -debug-y and -optimize-y flags,

but that was just for my own convenience.
Best Regards
Ingo Oeser
-

Thanks for feedback Ingo.

I just started a new thread were I outlined the ideas I have

for further extending (some may say uglifying) the kbuild syntax.

I will see the outcome of this thread before implmenting something.
PS. The implementation part is obviously trivial, the

hard part is the documentation :-)
	Sam

-

A couple of comments interspersed...
On 9/28/07, Erez Zadok <ezk@cs.sunysb.edu> wrote:

---
I think this "which" is non-restrictive, so it should have a comma

after it (I realize that's not part of your patch). It's also possible

to read it as restrictive, in which case "that" would be preferable.
---
"A number of people often like to..." is awkward. How about

"Developers sometimes..." or "Too many people..."
---
You might say "The kernel provides the macros likely()..."
---
The commas after "passed" and "thread" is unnecessary.
---
"...by the time it return." should be "...by the time it returns."
---
This "which" is restrictive; it would be preferable to use "that" instead.
---
The hyphen isn't necessary when the first word of the compount

adjective is an adverb ending in "-ly", so, just "seemingly rare"; or

switch to "apparently rare".
...
--

scott preece

-

The latter two?  Since there are only two presented I think there is no
--

Shawn

-

A few comments below...
---

~Randy

Phaedrus says that Quality is about caring.

-

The script calls checkpatch.pl on each file, and formats any error messages

to comply with standard compiler error messages:
	file_name:line_number:error_message
This is particularly useful when run from within a text editor which can

parse these error messages and show the user a buffer with the file in

question, placing the cursor on the offending line (e.g., Emacs's "M-x

next-error" command, bound by default to C-x `).
Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>

---

 scripts/check-coding-standards.pl |   59 +++++++++++++++++++++++++++++++++++++

 1 files changed, 59 insertions(+), 0 deletions(-)

 create mode 100755 scripts/check-coding-standards.pl
diff --git a/scripts/check-coding-standards.pl b/scripts/check-coding-standards.pl

new file mode 100755

index 0000000..a1ba597

--- /dev/null

+++ b/scripts/check-coding-standards.pl

@@ -0,0 +1,59 @@

+#!/usr/bin/perl -w

+# (c) 2007, Erez Zadok <ezk@cs.sunysb.edu>

+# Licensed under the terms of the GNU GPL License version 2

+#

+# Check one more more files for compliance with coding standards.

+# Outputs standard compiler-error format, one error per line, as follows

+#	filename:lineno:errormsg

+# This standard error message can be parsed by various tools, including

+# Emacs's M-x next-error

+#

+# Usage: check-coding-standards.pl file [files...]

+

+

+if (!defined($ARGV[0])) {

+    printf("Usage: $0 file [files...]\n");

+    exit(1);

+}

+

+$msg="";

+$lineno=0;

+foreach $file (@ARGV) {

+    die "$file: $!" unless (-f $file);

+    open(FILE, "diff -u /dev/null $file | perl scripts/checkpatch.pl --no-signoff - |") || die "$file: $!";

+    while (($line = <FILE>)) {

+	chop $line;

+	next if ($line =~ /^$/);

+	if ($line =~ /^ERROR: /) {

+	    $msg = $line;

+	    next;

+	}

+	if ($line =~ /^WARNING: /) {

+	    $msg = $line;

+	    next;

+	}

+	if ($line =~ /^CHECK: /) {

+	    $msg = $line;

+	    next;

+	}

+	if ($line =~ /^#/) {

+	    $lineno = (split(/:/, $line))[3];

+	    next;

+	}

+	if ($line =...
[ message continues ]

Hi Erez.
The concept is wrong here. If we want checkpatch to generate

message in standard gcc format then we should fix checkpatch and

not add a wrapper script around it.
For checkpatch related patches please forward them to the checkpatch

maintainers.
From MAINTAINERS:
CHECKPATCH

P:      Andy Whitcroft

M:      apw@shadowen.org

P:      Randy Dunlap

M:      rdunlap@xenotime.net

P:      Joel Schopp

M:      jschopp@austin.ibm.com

S:      Supported
	Sam

-

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>

---

 scripts/checkpatch.pl |    1 +

 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl

index dae7d30..ecbb030 100755

--- a/scripts/checkpatch.pl

+++ b/scripts/checkpatch.pl

@@ -33,6 +33,7 @@ if ($#ARGV < 0) {

 	print "version: $V\n";

 	print "options: -q           => quiet\n";

 	print "         --no-tree    => run without a kernel tree\n";

+	print "         --no-signoff => don't check for signed-off-by\n";

 	exit(1);

 }
--

1.5.2.2
-