Messages in this thread | | | Date | Wed, 25 Sep 2013 21:11:24 +0100 | From | Al Viro <> | Subject | Re: [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.
| |