login
Search
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
-
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
-
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 =~ /^\+/) {
+ ...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 -
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) +EXTRA_CFLAGS += -DDEBUG +endif + +In this way, you ...
A few comments below... --- ~Randy Phaedrus says that Quality is about caring. -
The latter two? Since there are only two presented I think there is no -- Shawn -
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 -
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 -
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 -
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. -
--- ~Randy Phaedrus says that Quality is about caring. -
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. -
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 ======================================================================== -
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 ...
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 -
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.
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 ;)
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 -
And yet people want to give license for maintainers to whine about parenthesis in printk's? Wow. - Ted -
... 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.
