lkml.org 
[lkml]   [2013]   [Sep]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] checkpatch: Find CamelCase definitions of struct/union/enum
On Wed, Sep 25, 2013 at 12:22:00PM -0700, Joe Perches wrote:
> It's pretty obvious from fs/binfmt_misc.c that you have
> your own taste.
>
> $ scripts/checkpatch.pl -f --strict fs/binfmt_misc.c
> [...]
> total: 45 errors, 39 warnings, 10 checks, 725 lines checked

*snort*

Most of those are whitespace noise, mostly having nothing whatsoever
to do with me or my taste. The rest... Let's see:

linux/uaccess.h instead of asm/uaccess.h. Reasonable enough these
days.

Several instances of CamelCase that isn't. Discussed upthread.

"WARNING: do not add new typedefs". Agreed for non-locals, strongly
disagreed in cases like that.

CHECK: Blank lines aren't necessary after an open brace '{'
#137: FILE: binfmt_misc.c:137:
+ if (fmt->flags & MISC_FMT_OPEN_BINARY) {
+

Agreed, ain't mine...

CHECK: braces {} should be used on all arms of this statement
#187: FILE: binfmt_misc.c:187:
+ if (fmt->flags & MISC_FMT_CREDENTIALS) {
[...]
+ } else
[...]

Umm... Again, that's not mine and while I agree in principle, in this case
I'm not sure it's worth bothering.

ERROR: switch and case should be at the same indent
#245: FILE: binfmt_misc.c:245:
+ switch (*p) {
+ case 'P':
[...]
+ case 'O':
[...]
+ case 'C':
[...]
+ default:

Not mine. I agree about indentation level, but there's more serious
style problem with that sucker - while (cont) thing would be better off
as while (1) { ... { ... default: return p; } }

CHECK: multiple assignments should be avoided
#291: FILE: binfmt_misc.c:291:
+ p = buf = (char *)e + sizeof(Node);

I hate it when they get religion. Should be avoided because of...?

A lot of complaints about
+ switch (*p++) {
+ case 'E': e->flags = 1<<Enabled; break;
+ case 'M': e->flags = (1<<Enabled) | (1<<Magic); break;
+ default: goto Einval;
and several other switches like that. IMO in a situation when all cases are
short, this kind of layout is OK.

WARNING: simple_strtoul is obsolete, use kstrtoul instead
#323: FILE: binfmt_misc.c:323:
+ e->offset = simple_strtoul(p, &p, 10);
Makes sense these days...

Several ones like this:
WARNING: braces {} are not necessary for single statement blocks
#438: FILE: binfmt_misc.c:438:
+ if (e->flags & MISC_FMT_PRESERVE_ARGV0) {
+ *dp++ = 'P';
+ }
Matter of taste... I probably would've skipped {} here, but it's not something
I'd bother changing in somebody else's patch (and it hadn't passed through
my hands, BTW - hist.git seems to indicate that it was passed by akpm; guess
he also hadn't bothered changing that)

CHECK: multiple assignments should be avoided
#481: FILE: binfmt_misc.c:481:
+ inode->i_atime = inode->i_mtime = inode->i_ctime =
I *do* hate it when they get religion.

ERROR: do not use assignment in if condition
#522: FILE: binfmt_misc.c:522:
+ if (!(page = (char *)__get_free_page(GFP_KERNEL)))
Ditto.


Overall: checkpatch.pl has produced 2 reasonable suggestions and pointed to
a place that is badly written for reasons unrelated to ones mentioned in
the output. That, along with some amount of BS was buried under tons of noise
about whitespace.


\
 
 \ /
  Last update: 2013-09-25 22:41    [W:0.091 / U:0.968 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site