Re: + checkpatch-add-check-for-use-of-sizeof-without-parenthesis.patchadded 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/