Re: + checkpatch-add-check-for-use-of-sizeof-without-parenthesis.patch added to -mm tree

Re: + checkpatch-add-check-for-use-of-sizeof-without-parenthesis.patch added to -mm tree

[David Rientjes]

On Mon, 9 Jul 2012, akpm@linux-foundation.org wrote:

> From: Joe Perches <joe@perches.com>
> Subject: checkpatch: Add acheck for use of sizeof without parenthesis
> 
> Kernel style uses parenthesis around sizeof.
> 

Nack, there's a difference between "sizeof *task" and 
"sizeof(struct task_struct)".  The former operates on a unary expression 
and the latter operates on a type.  There are over 1000 occurrences in the 
kernel where the sizeof operator, the former, is used on a unary 
expression.

Re: + checkpatch-add-check-for-use-of-sizeof-without-parenthesis.patch added to -mm tree

[Joe Perches]

On Mon, 2012-07-09 at 15:23 -0700, David Rientjes wrote:
> On Mon, 9 Jul 2012, akpm@linux-foundation.org wrote:
> 
> > From: Joe Perches <joe@perches.com>
> > Subject: checkpatch: Add acheck for use of sizeof without parenthesis
> > 
> > Kernel style uses parenthesis around sizeof.
> > 
> 
> Nack, there's a difference between "sizeof *task" and 
> "sizeof(struct task_struct)".  The former operates on a unary expression 
> and the latter operates on a type.  There are over 1000 occurrences in the 
> kernel where the sizeof operator, the former, is used on a unary 
> expression.

Huh?  Maybe I misunderstand you.

$ cat sizeof.c
#include <stdio.h>
#include <stdlib.h>
#include <strings.h>

struct foo {
	int bar[20];
	char *baz;
};

int main(int argc, char **argv)
{
	struct foo bar;
	struct foo *baz;

	printf("1: %zu\n", sizeof(struct foo));
	printf("2: %zu\n", sizeof bar);
	printf("3: %zu\n", sizeof *baz); 

	return 0;
}

$ gcc sizeof.c
$ ./a.out
1: 84
2: 84
3: 84

> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: + checkpatch-add-check-for-use-of-sizeof-without-parenthesis.patch added to -mm tree

[David Rientjes]

On Mon, 9 Jul 2012, Joe Perches wrote:

> Huh?  Maybe I misunderstand you.
> 
> $ cat sizeof.c
> #include <stdio.h>
> #include <stdlib.h>
> #include <strings.h>
> 
> struct foo {
> 	int bar[20];
> 	char *baz;
> };
> 
> int main(int argc, char **argv)
> {
> 	struct foo bar;
> 	struct foo *baz;
> 
> 	printf("1: %zu\n", sizeof(struct foo));

Parenthesis are required for type names such as this and is normally 
written as "sizeof (struct foo)" in gcc, glibc, the C99 standard, etc., 
but the Linux coding style asks that no space is introduced.

> 	printf("2: %zu\n", sizeof bar);

This is a unary expression and no parenthesis are required either by the 
C99 standard nor the Linux coding style and there are over 1000 
occurrences where there are no parenthesis for unary operators currently 
in the kernel.

So, nack, don't start enforcing your own coding style and preferences in 
checkpatch.pl.

Re: + checkpatch-add-check-for-use-of-sizeof-without-parenthesis.patch added to -mm tree

[Joe Perches]

On Mon, 2012-07-09 at 15:55 -0700, David Rientjes wrote:
> So, nack, don't start enforcing your own coding style and preferences in 
> checkpatch.pl.

Not just my opinion.

https://lkml.org/lkml/2008/12/23/138
--------------------------------------------------------
Date: Tue, 23 Dec 2008 10:08:50 -0800 (PST)
From: Linus Torvalds <>
[]
Another example of this is "sizeof". The kernel universally (I hope) has 
parenthesis around the sizeof argument, even though it's clearly not 
required by the C language. 

It's a coding standard. 
--------------------------------------------------------

cheers, Joe

Re: + checkpatch-add-check-for-use-of-sizeof-without-parenthesis.patch added to -mm tree

[David Rientjes]

On Mon, 9 Jul 2012, Joe Perches wrote:

> > So, nack, don't start enforcing your own coding style and preferences in 
> > checkpatch.pl.
> 
> Not just my opinion.
> 
> https://lkml.org/lkml/2008/12/23/138
> --------------------------------------------------------
> Date: Tue, 23 Dec 2008 10:08:50 -0800 (PST)
> From: Linus Torvalds <>
> []
> Another example of this is "sizeof". The kernel universally (I hope) has 
> parenthesis around the sizeof argument, even though it's clearly not 
> required by the C language. 
> 

Well, let's add Linus to the cc then because it's certainly not a C 
standard.  The sizeof operator requires parenthesis for type names, you 
can't do "sizeof unsigned long", for example, it requires 
"sizeof (unsigned long)".  All other unary operators do not need the 
parenthesis by ANY C standard.

Documentation/CodingStyle does not ask for the parenthesis to be added 
just like it doesn't ask for parenthesis to do things like (i++); which is 
another unary operator.  You would complain that (i++) has unnecessary 
parenthesis, and I claim the same for sizeof (*foo).

The fact that the kernel has over 1000 occurrences of "sizeof ..." without 
parenthesis shows that people actually know what _is_ standard, otherwise 
you'd get an error at compile time.  Taking just unary operators with 
pointers as an example, it spans the entire kernel:

$ grep -lr 'sizeof \*' * | cut -f1 -d / | uniq
arch
crypto
Documentation
drivers
fs
include
ipc
kernel
lib
net
security
sound
tools

So, for people like me who don't use checkpatch.pl at all, please modify 
Documentation/CodingStyle and get Linus to merge it to add this as a 
stylistic preference.  Saying it's a standard, though, is wrong.

Re: + checkpatch-add-check-for-use-of-sizeof-without-parenthesis.patch added to -mm tree

[Joe Perches]

On Mon, 2012-07-09 at 16:47 -0700, David Rientjes wrote:
> On Mon, 9 Jul 2012, Joe Perches wrote:
> 
> > > So, nack, don't start enforcing your own coding style and preferences in 
> > > checkpatch.pl.
> > 
> > Not just my opinion.
> > 
> > https://lkml.org/lkml/2008/12/23/138
> > --------------------------------------------------------
> > Date: Tue, 23 Dec 2008 10:08:50 -0800 (PST)
> > From: Linus Torvalds <>
> > []
> > Another example of this is "sizeof". The kernel universally (I hope) has 
> > parenthesis around the sizeof argument, even though it's clearly not 
> > required by the C language. 
> > 
> 
> Well, let's add Linus to the cc then because it's certainly not a C 
> standard.  The sizeof operator requires parenthesis for type names, you 
> can't do "sizeof unsigned long", for example, it requires 
> "sizeof (unsigned long)".  All other unary operators do not need the 
> parenthesis by ANY C standard.
> 
> Documentation/CodingStyle does not ask for the parenthesis to be added 
> just like it doesn't ask for parenthesis to do things like (i++); which is 
> another unary operator.

CodingStyle already does suggest parenthesis around sizeof

3.1:  Spaces

Linux kernel style for use of spaces depends (mostly) on
function-versus-keyword usage.  Use a space after (most) keywords.  The
notable exceptions are sizeof, typeof, alignof, and __attribute__, which look
somewhat like functions (and are usually used with parentheses in Linux,
although they are not required in the language, as in: "sizeof info" after
"struct fileinfo info;" is declared).

So use a space after these keywords:
	if, switch, case, for, do, while
but not with sizeof, typeof, alignof, or __attribute__.  E.g.,
	s = sizeof(struct file);

and

Chapter 14: Allocating memory
[]
The preferred form for passing a size of a struct is the following:

	p = kmalloc(sizeof(*p), ...);

Re: + checkpatch-add-check-for-use-of-sizeof-without-parenthesis.patch added to -mm tree

[David Rientjes]

On Mon, 9 Jul 2012, Joe Perches wrote:

> CodingStyle already does suggest parenthesis around sizeof
> 
> 3.1:  Spaces
> 
> Linux kernel style for use of spaces depends (mostly) on
> function-versus-keyword usage.  Use a space after (most) keywords.  The
> notable exceptions are sizeof, typeof, alignof, and __attribute__, which look
> somewhat like functions (and are usually used with parentheses in Linux,
> although they are not required in the language, as in: "sizeof info" after
> "struct fileinfo info;" is declared).
> 
> So use a space after these keywords:
> 	if, switch, case, for, do, while
> but not with sizeof, typeof, alignof, or __attribute__.  E.g.,
> 	s = sizeof(struct file);
> 

This doesn't suggest parenthesis for sizeof at all times, it's saying that 
Linux doesn't use the gcc, glibc, C99 way of doing "sizeof (struct file)" 
for type names as I've already said three times and rather prefers 
"sizeof(struct file)".  That's fine.

I'm talking about the C99 specification that says the sizeof operator act 
on unary expressions, i.e. not type names that you keep talking about and 
apparently don't understand the difference between.  Casts are done with 
type names, you can't do "unsigned long ptr", you do "(unsigned long)ptr".  
Sizeof operators using type names do the same thing: 
"sizeof (unsigned long)".  The space is just a stylistic difference.

You may not understand the difference between a type and a unary 
expression, but other C programmers do, i.e. those who have implemented 
the over 1000 places in the kernel that already do this because they 
learned from K&R.  I guarantee you that those who learned from K&R don't 
think sizeof(unsigned long) "looks like a function".

And typeof is a gnu extension, it's not part of the C99 standard.

> and
> 
> Chapter 14: Allocating memory
> []
> The preferred form for passing a size of a struct is the following:
> 
> 	p = kmalloc(sizeof(*p), ...);
> 

You realize your patch doesn't catch kmalloc(sizeof *p, ...) since it's 
checking for lval and the unary operator '*' is not considered part of an 
identifier, right?  Perhaps you should propose a patch that actually works 
first because yours is busted.

Re: + checkpatch-add-check-for-use-of-sizeof-without-parenthesis.patch added to -mm tree

[Joe Perches]

On Mon, 2012-07-09 at 18:50 -0700, David Rientjes wrote:
> On Mon, 9 Jul 2012, Joe Perches wrote:
> 
> > CodingStyle already does suggest parenthesis around sizeof
> > 
> > 3.1:  Spaces
> > 
> > Linux kernel style for use of spaces depends (mostly) on
> > function-versus-keyword usage.  Use a space after (most) keywords.  The
> > notable exceptions are sizeof, typeof, alignof, and __attribute__, which look
> > somewhat like functions (and are usually used with parentheses in Linux,
> > although they are not required in the language, as in: "sizeof info" after
> > "struct fileinfo info;" is declared).
> > 
> > So use a space after these keywords:
> > 	if, switch, case, for, do, while
> > but not with sizeof, typeof, alignof, or __attribute__.  E.g.,
> > 	s = sizeof(struct file);
> > 
> 
> This doesn't suggest parenthesis for sizeof at all times,

Depends on interpretation.  Linus' email from 2008 is pretty clear.

> it's saying that 
> Linux doesn't use the gcc, glibc, C99 way of doing "sizeof (struct file)" 
> for type names as I've already said three times and rather prefers 
> "sizeof(struct file)".  That's fine.
> 
> I'm talking about the C99 specification that says the sizeof operator act 
> on unary expressions, i.e. not type names that you keep talking about and 
> apparently don't understand the difference between.  Casts are done with 
> type names, you can't do "unsigned long ptr", you do "(unsigned long)ptr".  
> Sizeof operators using type names do the same thing: 
> "sizeof (unsigned long)".  The space is just a stylistic difference.
> 
> You may not understand the difference between a type and a unary 
> expression, but other C programmers do, i.e. those who have implemented 
> the over 1000 places in the kernel that already do this because they 
> learned from K&R.

I don't really care what style a large block of code
uses.  I care that it mostly has the same form.

fyi:  I learned C from K&R and Bill Joy in the mid 70's.

> I guarantee you that those who learned from K&R don't 
> think sizeof(unsigned long) "looks like a function".

Tell that to Linus. He wrote the email I referenced
which you so apparently blithely elided.

Repeating it:

"Another example of this is "sizeof". The kernel universally (I hope) has 
parenthesis around the sizeof argument, even though it's clearly not 
required by the C language."

> And typeof is a gnu extension, it's not part of the C99 standard.

Irrelevant as Linux uses it.

> You realize your patch doesn't catch kmalloc(sizeof *p, ...) since it's 
> checking for lval and the unary operator '*' is not considered part of an 
> identifier, right?

Oh look, useful feedback instead of useless carping about
how much I know or don't know c.

> Perhaps you should propose a patch that actually works 
> first because yours is busted.

I have one here.  I'll forward it soonish.

Re: + checkpatch-add-check-for-use-of-sizeof-without-parenthesis.patch added to -mm tree

[David Rientjes]

On Mon, 9 Jul 2012, Joe Perches wrote:

> I don't really care what style a large block of code
> uses.  I care that it mostly has the same form.
> 

Same form??  The sizeof operator has two forms depending on whether it's a 
unary expression or a type as specified by the standard.

The issue here is that you're mandating they all use the same form because 
you're quoting an email from Linus four years ago that you dug up but 
isn't required in the coding style and is already used in over 1000 places 
in the kernel.

If you want the output of checkpatch.pl to be useful, I would think you 
would want to eliminate this kind of garbage.

> > I guarantee you that those who learned from K&R don't 
> > think sizeof(unsigned long) "looks like a function".
> 
> Tell that to Linus. He wrote the email I referenced
> which you so apparently blithely elided.
> 
> Repeating it:
> 
> "Another example of this is "sizeof". The kernel universally (I hope) has 
> parenthesis around the sizeof argument, even though it's clearly not 
> required by the C language."
> 

He's obviously addressing a single form of the sizeof operator, i.e. those 
on unary expressions; sizeof used on a type CLEARLY DOES require the 
parenthesis.

Anyway, since this is my last email on the matter since I've already 
showed how your patch is completely busted, you're talking purely about 
style preferences here.  Don't convolute that by talking about the C 
standard or Linus' email where he says "it's a coding standard."  It's 
not.  I've layed out the two forms of the sizeof operator in every email 
I've written in this thread.  If you want to enforce a _style_ preference 
because you saw an email four years ago that Linus wrote, then add it to 
CodingStyle first and get him to mandate it.  That's how you effect a 
kernel-wide change; the _only_ person I know that uses checkpatch is 
Andrew and you'll notice the mm directory is the one where "sizeof ..." 
isn't used so just changing checkpatch here won't do much good.

Thanks and have a nice day.

Re: + checkpatch-add-check-for-use-of-sizeof-without-parenthesis.patch added to -mm tree

[Joe Perches]

On Mon, 2012-07-09 at 19:21 -0700, David Rientjes wrote:
> On Mon, 9 Jul 2012, Joe Perches wrote:
> 
> > I don't really care what style a large block of code
> > uses.  I care that it mostly has the same form.

> Same form??  The sizeof operator has two forms depending on whether it's a 
> unary expression or a type as specified by the standard.

> The issue here is that you're mandating they all use the same form because 
> you're quoting an email from Linus four years ago that you dug up but 
> isn't required in the coding style and is already used in over 1000 places 
> in the kernel.

$ git grep -E "\bsizeof\s*\*"|wc -l
935
$ git grep -E "\bsizeof\s*\(\s*\*"|wc -l
12762

> If you want the output of checkpatch.pl to be useful, I would think you 
> would want to eliminate this kind of garbage.

You are using high emotion words for little purpose.

checkpatch is useful, but it's not all that useful for those
quite familiar with kernel style.

Except maybe to generate flame emails...

It does have some use for reviewing patches.

> > "Another example of this is "sizeof". The kernel universally (I hope) has 
> > parenthesis around the sizeof argument, even though it's clearly not 
> > required by the C language."
> 
> He's obviously addressing a single form of the sizeof operator, i.e. those 
> on unary expressions; sizeof used on a type CLEARLY DOES require the 
> parenthesis.

That's one opinion, though I doubt it's his.
<shrug>  Maybe he'll reply.  (edit: he did)

> you're talking purely about style preferences here.

_All_ of checkpatch is style preference.
None if it is a mandate.  Those that care to use it can.
You can ignore it.  I don't mind.

Otherwise, just read and write the code and do what you
think best.  I'm not a particular style zealot.  I'm not
going to nack a patch just because you or anyone else uses
a style that isn't the predominate one.

cheers, Joe

Re: + checkpatch-add-check-for-use-of-sizeof-without-parenthesis.patch added to -mm tree

[Linus Torvalds]

On Mon, Jul 9, 2012 at 6:50 PM, David Rientjes <rientjes@google.com> wrote:
>
> This doesn't suggest parenthesis for sizeof at all times

sizeof without parenthesis is an abomination, and should never be used.

Sure, you don't need to have the parenthesis (except when you do - for
actual types), but it's a parsing oddity.

The sane solution is: just add the f*cking parenthesis, and don't use
the parsing oddity.

The parenthesis are *required* when it is a type, and they are a nice
clarification (and makes the code easier to read) when it's an
expression. Not having them is insane, because it just makes it clear
how odd the parsing rules are for the two different cases.

And talking about "prefix operators" is a moronic thing to do. It does
*not* look like a prefix operator, and it does not even *act* like a
prefix operator (look at types, where it really does require the
parenthesis). But most importantly, it's not how sane people think
about it.

Think of it as a function, and get over your idiotic pissing match
over how long you've both known C. That's irrelevant. It's a C builtin
function with (unnecessarily) odd parsing rules that the kernel tries
to standardize. The fact that it can take a type is the least odd part
of it (there are other built-in C extensions that look like functions
and do special things with the arguments they get -
__builtin_constant_p(), __builtin_choose_expr() etc - they don't
evaluate the *value* of the argument either).

Btw, the spacing rule is separate, and the "3.1 Spaces" thing was
added later and is in fact somewhat misleading. The spacing rules have
nothing to do with "function-vs-keyword", and everything to do with
"function-vs-control-flow". So "sizeof()" is not actually an exception
at all, like the docs state: it's perfectly regular. It looks and acts
as a function, not as a control flow operation.

What makes if/while/else/do/for stand out is that they are the native
C control flow operators, and they have spaces around them. They fact
that they are keywords is irrelevant. OF COURSE they are keywords,
since they are the native control flow ops, but that is not why they
have special spacing rules. The reason they have special spacing rules
is that

    for (x; y; z) {

is a control flow construct, while

    function(x, y, z);

is just a normal function expression. And in *no* case do we put
spaces after the parenthesis, we do it after the comma (or semicolon
for "for (a; b; c)").

We do have some control flow macros ("list_for_each()" etc), and they
could logically have the space, but hey, they are also normal macros,
so whatever. The kernel special-cases the native control-flow stuff,
not the random control flow macro oddities we've created for special
occasions.

                Linus

Re: + checkpatch-add-check-for-use-of-sizeof-without-parenthesis.patch added to -mm tree

[David Rientjes]

On Mon, 9 Jul 2012, Linus Torvalds wrote:

> sizeof without parenthesis is an abomination, and should never be used.
> 
> Sure, you don't need to have the parenthesis (except when you do - for
> actual types), but it's a parsing oddity.
> 
> The sane solution is: just add the f*cking parenthesis, and don't use
> the parsing oddity.
> 
> The parenthesis are *required* when it is a type, and they are a nice
> clarification (and makes the code easier to read) when it's an
> expression. Not having them is insane, because it just makes it clear
> how odd the parsing rules are for the two different cases.
> 

It's only strange looking because Documentation/CodingStyle doesn't use 
the gcc, glibc, C99, K&R, etc, style of doing "sizeof (struct foo)" and 
rather requires "sizeof(struct foo)".

> Think of it as a function, and get over your idiotic pissing match
> over how long you've both known C. That's irrelevant. It's a C builtin
> function with (unnecessarily) odd parsing rules that the kernel tries
> to standardize. The fact that it can take a type is the least odd part
> of it (there are other built-in C extensions that look like functions
> and do special things with the arguments they get -
> __builtin_constant_p(), __builtin_choose_expr() etc - they don't
> evaluate the *value* of the argument either).
> 

The only thing I've asked for is that something like this be added to 
Documentation/CodingStyle and not just in checkpatch.pl since hardly 
anybody uses that perl script.  You've chosen to mandate 
"sizeof(struct foo)" over "sizeof (struct foo)" in that document, which 
makes parenthesis for all unary expressions of the sizeof operator 
stylistically better as well, so it's only reasonable to ask that it 
mandates them for all unary expressions of sizeof.  It's your kernel.  
(That will be an exception for sizeof unary expressions, though, since we 
certainly don't want to mandate (i++).)

So, as I've been saying the whole thread, please either mandate it kernel-
wide in the CodingStyle or don't do it in checkpatch.  Checkpatch 
continually acts as a backdoor way of people enforcing their own 
preferences over others.

[PATCH V2] checkpatch: Add check for use of sizeof without parenthesis

[Joe Perches]

Kernel style uses parenthesis around sizeof.

Signed-off-by: Joe Perches <joe@perches.com>
---
Add check that works for sizeof *foo as well as sizeof foo

 scripts/checkpatch.pl |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7190f95..72c1803 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3265,6 +3265,12 @@ sub process {
 			     "sizeof(& should be avoided\n" . $herecurr);
 		}
 
+# check for sizeof without parenthesis
+		if ($line =~ /\bsizeof\s+((?:\*\s*|)$Lval|$Type(?:\s+$Lval|))/) {
+			WARN("SIZEOF_PARENTHESIS",
+			     "sizeof $1 should be sizeof($1)\n" . $herecurr);
+		}
+
 # check for line continuations in quoted strings with odd counts of "
 		if ($rawline =~ /\\$/ && $rawline =~ tr/"/"/ % 2) {
 			WARN("LINE_CONTINUATIONS",