Mixed space/tab indentation is wrong. Ironically this is what the coding
style document actually shows ;)
Signed-off-by: Auke Kok <[email protected]>
---
Documentation/CodingStyle | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
index afc2867..f518395 100644
--- a/Documentation/CodingStyle
+++ b/Documentation/CodingStyle
@@ -13,7 +13,7 @@ and NOT read it. Burn them, it's a great symbolic gesture.
Anyway, here goes:
- Chapter 1: Indentation
+ Chapter 1: Indentation
Tabs are 8 characters, and thus indentations are also 8 characters.
There are heretic movements that try to make indentations 4 (or even 2!)
@@ -71,6 +71,8 @@ used for indentation, and the above example is deliberately broken.
Get a decent editor and don't leave whitespace at the end of lines.
+Don't put spaces before tabs or mix them.
+
Chapter 2: Breaking long lines and strings
Several standards have been established on how to format tests and use
NULL/false/true tests.
Signed-off-by: Auke Kok <[email protected]>
---
Documentation/CodingStyle | 51 +++++++++++++++++++++++++++++++++++----------
1 files changed, 40 insertions(+), 11 deletions(-)
diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
index f518395..3635b38 100644
--- a/Documentation/CodingStyle
+++ b/Documentation/CodingStyle
@@ -393,7 +393,7 @@ int fun(int a)
int result = 0;
char *buffer = kmalloc(SIZE);
- if (buffer == NULL)
+ if (!buffer)
return -ENOMEM;
if (condition1) {
@@ -409,7 +409,36 @@ out:
return result;
}
- Chapter 8: Commenting
+ Chapyer 8: Tests
+
+Testing return values from function calls can get complex when you need
+to re-use the value later on. You should store the value before doing
+any tests on it. Never assign values inside a condition to another
+variable.
+
+ err = test_something();
+ if (err) {
+ printk(KERN_ERR "Error: test_something() failed\n");
+ return err;
+ }
+
+If you give your variables and pointers good names, there is never a need
+to compare the value stored in that variable to NULL or true/false, so
+omit all that and keep it short.
+
+ ptr = s->next;
+ if (!ptr)
+ return;
+
+ v = (read_byte(register));
+ if (v & mask)
+ return;
+
+ if (is_prime(number))
+ return 0;
+
+
+ Chapter 9: Commenting
Comments are good, but there is also a danger of over-commenting. NEVER
try to explain HOW your code works in a comment: it's much better to
@@ -449,7 +478,7 @@ multiple data declarations). This leaves you room for a small comment on each
item, explaining its use.
- Chapter 9: You've made a mess of it
+ Chapter 10: You've made a mess of it
That's OK, we all do. You've probably been told by your long-time Unix
user helper that "GNU emacs" automatically formats the C sources for
@@ -497,7 +526,7 @@ re-formatting you may want to take a look at the man page. But
remember: "indent" is not a fix for bad programming.
- Chapter 10: Configuration-files
+ Chapter 11: Configuration-files
For configuration options (arch/xxx/Kconfig, and all the Kconfig files),
somewhat different indentation is used.
@@ -522,7 +551,7 @@ support for file-systems, for instance) should be denoted (DANGEROUS), other
experimental options should be denoted (EXPERIMENTAL).
- Chapter 11: Data structures
+ Chapter 12: Data structures
Data structures that have visibility outside the single-threaded
environment they are created and destroyed in should always have
@@ -553,7 +582,7 @@ Remember: if another thread can find your data structure, and you don't
have a reference count on it, you almost certainly have a bug.
- Chapter 12: Macros, Enums and RTL
+ Chapter 13: Macros, Enums and RTL
Names of macros defining constants and labels in enums are capitalized.
@@ -608,7 +637,7 @@ The cpp manual deals with macros exhaustively. The gcc internals manual also
covers RTL which is used frequently with assembly language in the kernel.
- Chapter 13: Printing kernel messages
+ Chapter 14: Printing kernel messages
Kernel developers like to be seen as literate. Do mind the spelling
of kernel messages to make a good impression. Do not use crippled
@@ -619,7 +648,7 @@ Kernel messages do not have to be terminated with a period.
Printing numbers in parentheses (%d) adds no value and should be avoided.
- Chapter 14: Allocating memory
+ Chapter 15: Allocating memory
The kernel provides the following general purpose memory allocators:
kmalloc(), kzalloc(), kcalloc(), and vmalloc(). Please refer to the API
@@ -638,7 +667,7 @@ from void pointer to any other pointer type is guaranteed by the C programming
language.
- Chapter 15: The inline disease
+ Chapter 16: The inline disease
There appears to be a common misperception that gcc has a magic "make me
faster" speedup option called "inline". While the use of inlines can be
@@ -665,7 +694,7 @@ appears outweighs the potential value of the hint that tells gcc to do
something it would have done anyway.
- Chapter 16: Function return values and names
+ Chapter 17: Function return values and names
Functions can return values of many different kinds, and one of the
most common is a value indicating whether the function succeeded or
@@ -699,7 +728,7 @@ result. Typical examples would be functions that return pointers; they use
NULL or the ERR_PTR mechanism to report failure.
- Chapter 17: Don't re-invent the kernel macros
+ Chapter 18: Don't re-invent the kernel macros
The header file include/linux/kernel.h contains a number of macros that
you should use, rather than explicitly coding some variant of them yourself.
Auke Kok wrote:
> +Don't put spaces before tabs or mix them.
Make it "Don't put spaces before tabs."
We do mix them if we combine tabs for indentation with spaces for alignment.
--
Stefan Richter
-=====-=-=== -=-= ==--=
http://arcgraph.de/sr/
Auke Kok wrote:
> +If you give your variables and pointers good names, there is never a need
> +to compare the value stored in that variable to NULL or true/false, so
> +omit all that and keep it short.
I agree with this in principle. But do we have to standardize it?
--
Stefan Richter
-=====-=-=== -=-= ==--=
http://arcgraph.de/sr/
Stefan Richter wrote:
> Auke Kok wrote:
>> +If you give your variables and pointers good names, there is never a need
>> +to compare the value stored in that variable to NULL or true/false, so
>> +omit all that and keep it short.
>
> I agree with this in principle. But do we have to standardize it?
yes, I think so. Not only does it remove useless fluff but it forces you to pick
your function and variable names decently. It really doesn't hurt to mention it
especially when you see how many drivers have copied bad style over without
knowing better. Now we can refer them to the CodingStyle doc right away.
Auke
Stefan Richter wrote:
> Auke Kok wrote:
>> +Don't put spaces before tabs or mix them.
>
> Make it "Don't put spaces before tabs."
>
> We do mix them if we combine tabs for indentation with spaces for alignment.
It would probably be good to add a pointer to the cleanfile/cleanpatch
scripts here, too.
-hpa
H. Peter Anvin wrote:
> Stefan Richter wrote:
>> Auke Kok wrote:
>>> +Don't put spaces before tabs or mix them.
>> Make it "Don't put spaces before tabs."
>>
>> We do mix them if we combine tabs for indentation with spaces for alignment.
>
> It would probably be good to add a pointer to the cleanfile/cleanpatch
> scripts here, too.
ahh yes, I'll incorporate that.
BTW, would it be possible for cleanfile/cleanpatch to dump warnings to stderr
for lines exceeding 80 characters? I think that would really be useful...
Auke
Kok, Auke wrote:
>
> ahh yes, I'll incorporate that.
>
> BTW, would it be possible for cleanfile/cleanpatch to dump warnings to
> stderr for lines exceeding 80 characters? I think that would really be
> useful...
>
Should be trivial enough to do, although it probably should be an option.
-hpa
On May 25 2007 10:25, Auke Kok wrote:
>diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
>index f518395..3635b38 100644
>--- a/Documentation/CodingStyle
>+++ b/Documentation/CodingStyle
>@@ -393,7 +393,7 @@ int fun(int a)
> int result = 0;
> char *buffer = kmalloc(SIZE);
>
>- if (buffer == NULL)
>+ if (!buffer)
> return -ENOMEM;
Please don't do this. With ==NULL/!=NULL, it is clear what
<randomvariable> could be (integer or pointer) without needing
to look it up. It also reads quite strange: "if not buffer".
For bools ('adjectives' / 'is a'), it works, not so much for ptrs.
Hence:
>+If you give your variables and pointers good names, there is never a need
>+to compare the value stored in that variable to NULL or true/false, so
>+omit all that and keep it short.
>+ ptr = s->next;
>+ if (!ptr)
>+ return;
Not agreed.
>+
>+ v = (read_byte(register));
>+ if (v & mask)
>+ return;
well, yes.
>+ if (is_prime(number))
Yes.
And I'd also like to mention one rather special case where I'd rather
like to see ==0 than ! for clarity (!strcmp looks like !streq, so
one needs to look twice to get it):
if (!strcmp(hay, needle))
At least don't force the '!' doctrine.
Jan
--
Based in part on Auke's patch.
Signed-off-by: Jan Engelhardt <[email protected]>
---
Documentation/CodingStyle | 74 +++++++++++++++++++++++++++++++++++++++-------
1 file changed, 64 insertions(+), 10 deletions(-)
Index: linux-2.6.22-rc3/Documentation/CodingStyle
===================================================================
--- linux-2.6.22-rc3.orig/Documentation/CodingStyle
+++ linux-2.6.22-rc3/Documentation/CodingStyle
@@ -407,7 +407,61 @@ out:
return result;
}
- Chapter 8: Commenting
+ Chapyer 8: Tests
+
+Testing return values from function calls can get complex when you need
+to re-use the value later on. You should store the value before doing
+any tests on it. Do not assign values inside a condition to another
+variable.
+
+ err = test_something();
+ if (err) {
+ printk(KERN_ERR "Error: test_something() failed\n");
+ return err;
+ }
+
+Testing for a flag, as done in the following example, is redundant and
+can be shortened.
+
+ if ((v & GFP_KERNEL) == GFP_KERNEL)
+ return;
+
+should become
+
+ if (v & GFP_KERNEL)
+ return;
+
+The same goes for functions that return a bool:
+
+ if (is_prime(number) == true)
+ return 0;
+ if (is_prime(number) == false)
+ return 1;
+
+should be:
+
+ if (is_prime(number))
+ return 0;
+ if (!is_prime(number))
+ return 1;
+
+As far as pointers or functions returning an integer are concerned,
+using long form tests helps to distinguish between pointers and bools
+or functions returning boolean or integer, respectively.
+Examples are:
+
+ if (p == NULL)
+ return 1;
+ if (!p)
+ return 0;
+
+ if (strcmp(haystack, needle) == 0)
+ return 1;
+ if (!strcmp(haystack, needle))
+ return 0;
+
+
+ Chapter 9: Commenting
Comments are good, but there is also a danger of over-commenting. NEVER
try to explain HOW your code works in a comment: it's much better to
@@ -447,7 +501,7 @@ multiple data declarations). This leave
item, explaining its use.
- Chapter 9: You've made a mess of it
+ Chapter 10: You've made a mess of it
That's OK, we all do. You've probably been told by your long-time Unix
user helper that "GNU emacs" automatically formats the C sources for
@@ -495,7 +549,7 @@ re-formatting you may want to take a loo
remember: "indent" is not a fix for bad programming.
- Chapter 10: Kconfig configuration files
+ Chapter 11: Kconfig configuration files
For all of the Kconfig* configuration files throughout the source tree,
the indentation is somewhat different. Lines under a "config" definition
@@ -531,7 +585,7 @@ For full documentation on the configurat
Documentation/kbuild/kconfig-language.txt.
- Chapter 11: Data structures
+ Chapter 12: Data structures
Data structures that have visibility outside the single-threaded
environment they are created and destroyed in should always have
@@ -562,7 +616,7 @@ Remember: if another thread can find you
have a reference count on it, you almost certainly have a bug.
- Chapter 12: Macros, Enums and RTL
+ Chapter 13: Macros, Enums and RTL
Names of macros defining constants and labels in enums are capitalized.
@@ -617,7 +671,7 @@ The cpp manual deals with macros exhaust
covers RTL which is used frequently with assembly language in the kernel.
- Chapter 13: Printing kernel messages
+ Chapter 14: Printing kernel messages
Kernel developers like to be seen as literate. Do mind the spelling
of kernel messages to make a good impression. Do not use crippled
@@ -628,7 +682,7 @@ Kernel messages do not have to be termin
Printing numbers in parentheses (%d) adds no value and should be avoided.
- Chapter 14: Allocating memory
+ Chapter 15: Allocating memory
The kernel provides the following general purpose memory allocators:
kmalloc(), kzalloc(), kcalloc(), and vmalloc(). Please refer to the API
@@ -647,7 +701,7 @@ from void pointer to any other pointer t
language.
- Chapter 15: The inline disease
+ Chapter 16: The inline disease
There appears to be a common misperception that gcc has a magic "make me
faster" speedup option called "inline". While the use of inlines can be
@@ -674,7 +728,7 @@ appears outweighs the potential value of
something it would have done anyway.
- Chapter 16: Function return values and names
+ Chapter 17: Function return values and names
Functions can return values of many different kinds, and one of the
most common is a value indicating whether the function succeeded or
@@ -708,7 +762,7 @@ result. Typical examples would be funct
NULL or the ERR_PTR mechanism to report failure.
- Chapter 17: Don't re-invent the kernel macros
+ Chapter 18: Don't re-invent the kernel macros
The header file include/linux/kernel.h contains a number of macros that
you should use, rather than explicitly coding some variant of them yourself.
On 5/26/07, Jan Engelhardt <[email protected]> wrote:
>
> Based in part on Auke's patch.
>
> Signed-off-by: Jan Engelhardt <[email protected]>
>
> ---
> Documentation/CodingStyle | 74 +++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 64 insertions(+), 10 deletions(-)
>
> Index: linux-2.6.22-rc3/Documentation/CodingStyle
> ===================================================================
> --- linux-2.6.22-rc3.orig/Documentation/CodingStyle
> +++ linux-2.6.22-rc3/Documentation/CodingStyle
> @@ -407,7 +407,61 @@ out:
> return result;
> }
>
> - Chapter 8: Commenting
> + Chapyer 8: Tests
---
Fix the "Chapyer" spelling above.
However, I think this set of rules is not universally agreed to and
probably should not be added at this time. I'm not convinced that it
makes the code more readable.
scott
Jan Engelhardt wrote:
> + if (is_prime(number) == true)
> + return 0;
> + if (is_prime(number) == false)
> + return 1;
> +
> +should be:
> +
> + if (is_prime(number))
> + return 0;
> + if (!is_prime(number))
> + return 1;
> +
> +As far as pointers or functions returning an integer are concerned,
> +using long form tests helps to distinguish between pointers and bools
> +or functions returning boolean or integer, respectively.
> +Examples are:
> +
> + if (p == NULL)
> + return 1;
> + if (!p)
> + return 0;
> +
> + if (strcmp(haystack, needle) == 0)
> + return 1;
> + if (!strcmp(haystack, needle))
> + return 0;
The latter two examples seem odd. Didn't you mean the following?
if (p == NULL)
return 1;
if (p)
return 0;
if (strcmp(haystack, needle) == 0)
return 1;
if (strcmp(haystack, needle))
return 0;
Perhaps better:
if (p == NULL)
return NO_MEMORY;
if (p)
return MEMORY;
if (strcmp(haystack, needle) == 0)
return IS_SAME;
if (strcmp(haystack, needle))
return IS_DIFFERENT;
However, to follow your argument about non-boolean expressions, the
following would be more consequently going into your direction:
if (p == NULL)
return NO_MEMORY;
if (p != NULL)
return MEMORY;
if (strcmp(haystack, needle) == 0)
return IS_SAME;
if (strcmp(haystack, needle) != 0)
return IS_DIFFERENT;
I.e., why do the explicit comparison with 0 or NULL only when it is
tested for equality, but not when testing for inequality?
However, I agree with Scott Preece that these rules should be left out
of CodingStyle because they are contentious.
(Disclosure: I am personally used to "if (p)" and "if (!p)" tests of
pointers and many integer expressions, but I tend to the longer form in
less obvious cases like "if (strcmp(a, b) != 0)".)
--
Stefan Richter
-=====-=-=== -=-= ==-==
http://arcgraph.de/sr/
On May 27 2007 16:37, Stefan Richter wrote:
>Jan Engelhardt wrote:
>> + if (is_prime(number) == true)
>> + return 0;
>> + if (is_prime(number) == false)
>> + return 1;
>> +
>> +should be:
>> +
>> + if (is_prime(number))
>> + return 0;
>> + if (!is_prime(number))
>> + return 1;
>> +
>> +As far as pointers or functions returning an integer are concerned,
>> +using long form tests helps to distinguish between pointers and bools
>> +or functions returning boolean or integer, respectively.
>> +Examples are:
>> +
>> + if (p == NULL)
>> + return 1;
>> + if (!p)
>> + return 0;
>> +
>> + if (strcmp(haystack, needle) == 0)
>> + return 1;
>> + if (!strcmp(haystack, needle))
>> + return 0;
>
>The latter two examples seem odd. Didn't you mean the following?
See how much confusion it all makes!
Right, it was intended -- first the long form is shown and then the
shorter one (and "long form tests help to distinguish"):
if (p == NULL) /* this way please */
return 1;
if (!p) /* Everytime you shorten it, God kills a kitten */
return 0;
/* so perhaps don't do it if you love animals or
know someone who does. */
I seem to have forgotten more comments/explanation.
> if (p == NULL)
> return 1;
> if (p)
> return 0;
>
> if (strcmp(haystack, needle) == 0)
> return 1;
> if (strcmp(haystack, needle))
> return 0;
>
>Perhaps better:
>
> if (p == NULL)
> return NO_MEMORY;
> if (p)
> return MEMORY;
>
> if (strcmp(haystack, needle) == 0)
> return IS_SAME;
> if (strcmp(haystack, needle))
> return IS_DIFFERENT;
>
>However, to follow your argument about non-boolean expressions, the
>following would be more consequently going into your direction:
>
>I.e., why do the explicit comparison with 0 or NULL only when it is
>tested for equality, but not when testing for inequality?
>
>However, I agree with Scott Preece that these rules should be left out
>of CodingStyle because they are contentious.
>
>(Disclosure: I am personally used to "if (p)" and "if (!p)" tests of
>pointers and many integer expressions, but I tend to the longer form in
>less obvious cases like "if (strcmp(a, b) != 0)".)
Jan
--
Jan Engelhardt wrote:
> if (!p) /* Everytime you shorten it, God kills a kitten */
Very bad news for felines, judging from current coding practice.
--
Stefan Richter
-=====-=-=== -=-= ==-==
http://arcgraph.de/sr/
On May 27 2007 17:23, Stefan Richter wrote:
>Jan Engelhardt wrote:
>> if (!p) /* Everytime you shorten it, God kills a kitten */
>
>Very bad news for felines, judging from current coding practice.
Don't worry. There are far worse codingstyles around, like the GNU 'standard'.
(Indent is the premier element of any style.)
Jan
--
On 25/05/07, Auke Kok <[email protected]> wrote:
> Several standards have been established on how to format tests and use
> NULL/false/true tests.
>
Hmm, this may or may not be a good idea for CodingStyle, I'll leave
that up to others.
But, if you are going to renumber chapters then please also fix up the
references to those chapter numbers elsewhere in the file - like this
one
"... (for example as a means of replacing macros, see Chapter 12) ..."
After your patch that now points to the wrong chapter.
--
Jesper Juhl <[email protected]>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html
Jan Engelhardt wrote:
> On May 25 2007 10:25, Auke Kok wrote:
>> diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
>> index f518395..3635b38 100644
>> --- a/Documentation/CodingStyle
>> +++ b/Documentation/CodingStyle
>> @@ -393,7 +393,7 @@ int fun(int a)
>> int result = 0;
>> char *buffer = kmalloc(SIZE);
>>
>> - if (buffer == NULL)
>> + if (!buffer)
>> return -ENOMEM;
>
> Please don't do this. With ==NULL/!=NULL, it is clear what
> <randomvariable> could be (integer or pointer) without needing
> to look it up. It also reads quite strange: "if not buffer".
> For bools ('adjectives' / 'is a'), it works, not so much for ptrs.
> Hence:
>
>> +If you give your variables and pointers good names, there is never a need
>> +to compare the value stored in that variable to NULL or true/false, so
>> +omit all that and keep it short.
>
>> + ptr = s->next;
>> + if (!ptr)
>> + return;
>
> Not agreed.
that piece is a copy of mm/slab.c, and all over the core components of the
kernel (even fs/inode.c written by Linus). I strongly think that "== NULL"
doesn't add anything and that well-written functions and well-named variables
really do not need the extra fluff.
Auke
On 5/27/07, Kok, Auke <[email protected]> wrote:
> that piece is a copy of mm/slab.c, and all over the core components of the
> kernel (even fs/inode.c written by Linus). I strongly think that "== NULL"
> doesn't add anything and that well-written functions and well-named variables
> really do not need the extra fluff.
Yup, I we really don't use "== NULL" in core code that much. But I am
not convinced this should be in CodingStyle either. It's more of a
maintainer preference thing above all.
On Sat, 26 May 2007, Jan Engelhardt wrote:
> +Testing for a flag, as done in the following example, is redundant and
> +can be shortened.
> +
> + if ((v & GFP_KERNEL) == GFP_KERNEL)
> + return;
> +
> +should become
> +
> + if (v & GFP_KERNEL)
> + return;
This looks wrong to me. These two are only equivalent if the "flag" only
has 1 bit. And already here you fall into this trap:
#define GFP_KERNEL (__GFP_WAIT | __GFP_IO | __GFP_FS)
Thanks
Guennadi
---
Guennadi Liakhovetski