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

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.
--
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 [vger.kernel.org]
Please read the FAQ at [www.tux.org]

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 [vger.kernel.org]
> Please read the FAQ at [www.tux.org]
>



--
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 [vger.kernel.org]
Please read the FAQ at [www.tux.org]

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.
--
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 [vger.kernel.org]
Please read the FAQ at [www.tux.org]

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.

[lkml.org]
--------------------------------------------------------
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

--
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 [vger.kernel.org]
Please read the FAQ at [www.tux.org]

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.
>
> [lkml.org]
> --------------------------------------------------------
> 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.
--
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 [vger.kernel.org]
Please read the FAQ at [www.tux.org]

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.
> >
> > [lkml.org]
> > --------------------------------------------------------
> > 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), ...);


--
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 [vger.kernel.org]
Please read the FAQ at [www.tux.org]

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.
--
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 [vger.kernel.org]
Please read the FAQ at [www.tux.org]

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.

--
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 [vger.kernel.org]
Please read the FAQ at [www.tux.org]

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.
--
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 [vger.kernel.org]
Please read the FAQ at [www.tux.org]

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
--
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 [vger.kernel.org]
Please read the FAQ at [www.tux.org]

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.
--
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 [vger.kernel.org]
Please read the FAQ at [www.tux.org]

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

--
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 [vger.kernel.org]
Please read the FAQ at [www.tux.org]