[PATCH] checkpatch: Find CamelCase definitions of struct/union/enum

[PATCH] checkpatch: Find CamelCase definitions of struct/union/enum

[Joe Perches]

Checkpatch doesn't currently find CamelCase definitions
of structs, unions or enums.

Add that ability.

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c03e427..3d51710 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -442,8 +442,9 @@ sub seed_camelcase_file {
 		next if ($line !~ /(?:[A-Z][a-z]|[a-z][A-Z])/);
 		if ($line =~ /^[ \t]*(?:#[ \t]*define|typedef\s+$Type)\s+(\w*(?:[A-Z][a-z]|[a-z][A-Z])\w*)/) {
 			$camelcase{$1} = 1;
-		}
-	        elsif ($line =~ /^\s*$Declare\s+(\w*(?:[A-Z][a-z]|[a-z][A-Z])\w*)\s*[\(\[,;]/) {
+		} elsif ($line =~ /^\s*$Declare\s+(\w*(?:[A-Z][a-z]|[a-z][A-Z])\w*)\s*[\(\[,;]/) {
+			$camelcase{$1} = 1;
+		} elsif ($line =~ /^\s*(?:union|struct|enum)\s+(\w*(?:[A-Z][a-z]|[a-z][A-Z])\w*)\s*[;\{]/) {
 			$camelcase{$1} = 1;
 		}
 	}

Re: [PATCH] checkpatch: Find CamelCase definitions of struct/union/enum

[Al Viro]

On Tue, Sep 24, 2013 at 11:38:54AM -0700, Joe Perches wrote:
> Checkpatch doesn't currently find CamelCase definitions
> of structs, unions or enums.
> 
> Add that ability.

Fix your regex.  As it is, it catches the things that are not camelcase
by any stretch of definition - [A-Z][a-z]+ will get caught by that and
it's neither camelcase nor something that ought to be discouraged for
e.g. enum member names.

CamelCase refers to caps in the middle of a compound, used to visually
separate them.

Re: [PATCH] checkpatch: Find CamelCase definitions of struct/union/enum

[Joe Perches]

On Wed, 2013-09-25 at 16:24 +0100, Al Viro wrote:
> On Tue, Sep 24, 2013 at 11:38:54AM -0700, Joe Perches wrote:
> > Checkpatch doesn't currently find CamelCase definitions
> > of structs, unions or enums.
> > 
> > Add that ability.
> 
> Fix your regex.  As it is, it catches the things that are not camelcase
> by any stretch of definition - [A-Z][a-z]+ will get caught by that and
> it's neither camelcase nor something that ought to be discouraged for
> e.g. enum member names.
> 
> CamelCase refers to caps in the middle of a compound, used to visually
> separate them.

We disagree.

I think Propercase should be discouraged.
ie: Qdisc et al.

Anyway, this finds things like Qdisc in
headers and adds them to the "ignore" list.

Re: [PATCH] checkpatch: Find CamelCase definitions of struct/union/enum

[Al Viro]

On Wed, Sep 25, 2013 at 08:35:44AM -0700, Joe Perches wrote:

> We disagree.

Obviously.

> I think Propercase should be discouraged.
> ie: Qdisc et al.

Excuse me, but "Joe happens to think that something should be discouraged"
is not a problem.  "Joe uses checkpatch.pl as force multiplier, recruiting
hundreds of monkeys to enforce his personal preferences", OTOH, very much is.

You are calling for ban on any mixed-case identifiers.  I see at least three
cases where they can be legitimate:
	* labels a-la Enomem, etc.  I've been using those and I will
keep doing so, checkpatch.pl and its users be damned.
	* enum members, to distinguish those from defines (first letter
capitalized vs. all-caps).
	* (local) typedefs for structs; I really don't like their use for
anything non-local, but IMO they have their uses in cases like e.g.
fs/binfmt_misc.c

Re: [PATCH] checkpatch: Find CamelCase definitions of struct/union/enum

[Alexander Holler]

Am 25.09.2013 18:19, schrieb Al Viro:
> On Wed, Sep 25, 2013 at 08:35:44AM -0700, Joe Perches wrote:
>
>> We disagree.
>
> Obviously.
>
>> I think Propercase should be discouraged.
>> ie: Qdisc et al.
>
> Excuse me, but "Joe happens to think that something should be discouraged"
> is not a problem.  "Joe uses checkpatch.pl as force multiplier, recruiting
> hundreds of monkeys to enforce his personal preferences", OTOH, very much is.
>
> You are calling for ban on any mixed-case identifiers.  I see at least three
> cases where they can be legitimate:
> 	* labels a-la Enomem, etc.  I've been using those and I will
> keep doing so, checkpatch.pl and its users be damned.
> 	* enum members, to distinguish those from defines (first letter
> capitalized vs. all-caps).
> 	* (local) typedefs for structs; I really don't like their use for
> anything non-local, but IMO they have their uses in cases like e.g.
> fs/binfmt_misc.c

Besides that CamelCase is one character less long than camel_case.

I'm awaiting kernel developer uniforms. There are too many different 
black t-shirts around. ;)

Regards,

Alexander Holler

Re: [PATCH] checkpatch: Find CamelCase definitions of struct/union/enum

[Joe Perches]

On Wed, 2013-09-25 at 17:19 +0100, Al Viro wrote:
> On Wed, Sep 25, 2013 at 08:35:44AM -0700, Joe Perches wrote:
> > I think Propercase should be discouraged.
> > ie: Qdisc et al.
> 
> Excuse me, but "Joe happens to think that something should be discouraged"
> is not a problem.  "Joe uses checkpatch.pl as force multiplier, recruiting
> hundreds of monkeys to enforce his personal preferences", OTOH, very much is.

Hah.  I'm still looking to acquire minions not monkeys,
and the force seems weak in that one.  Enthralling the
wannabe thralls isn't working...

CamelCase uses are not reported by default.

So not to worry Al, CamelCase is a --strict (CHECK) test
and almost no one ever uses --strict.  

> You are calling for ban on any mixed-case identifiers.  I see at least three
> cases where they can be legitimate:

If I was calling for anything remotely like a "ban",
I'd try to add it to CodingStyle.

> 	* labels a-la Enomem, etc.  I've been using those and I will
> keep doing so, checkpatch.pl and its users be damned.

Good on you.

> 	* enum members, to distinguish those from defines (first letter
> capitalized vs. all-caps).

Shrug.  There aren't that many uses of that style.

> 	* (local) typedefs for structs; I really don't like their use for
> anything non-local, but IMO they have their uses in cases like e.g.
> fs/binfmt_misc.c

Fine by me.  Don't use checkpatch.  No stress from me.

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

Re: [PATCH] checkpatch: Find CamelCase definitions of struct/union/enum

[Theodore Ts'o]

On Wed, Sep 25, 2013 at 12:22:00PM -0700, Joe Perches wrote:
> 
> > 	* labels a-la Enomem, etc.  I've been using those and I will
> > keep doing so, checkpatch.pl and its users be damned.
> 
> Good on you.

I'm with Al.  This is just silly to add to checkpatch.

> Fine by me.  Don't use checkpatch.  No stress from me.

I still get some of your minions sending patches asking to "fix" ext4
from time to time.  It wastes everyone's time.

BTW, checkpatch barfs on pretty much everything in
linux/trace/events/*.h

						- Ted

Re: [PATCH] checkpatch: Find CamelCase definitions of struct/union/enum

[Joe Perches]

On Wed, 2013-09-25 at 15:30 -0400, Theodore Ts'o wrote:
> On Wed, Sep 25, 2013 at 12:22:00PM -0700, Joe Perches wrote:
> > 
> > > 	* labels a-la Enomem, etc.  I've been using those and I will
> > > keep doing so, checkpatch.pl and its users be damned.
> > 
> > Good on you.
> 
> I'm with Al.  This is just silly to add to checkpatch.
> 
> > Fine by me.  Don't use checkpatch.  No stress from me.
> 
> I still get some of your minions sending patches asking to "fix" ext4
> from time to time.  It wastes everyone's time.

ext4 still has several code style nits that could be
improved.  Up to you to do anything about them though.

no space after comma, pointer spaces, and split strings
are nits.

> BTW, checkpatch barfs on pretty much everything in
> linux/trace/events/*.h

include/trace/events/*.h

As I've said multiple times, checkpatch is a stupid
little tool that can be ignored by anyone with
different tastes.

And, shrug.  I think straining for alignment like below
doesn't really add much.

$ ./scripts/checkpatch.pl -f include/trace/events/jbd.h
ERROR: space prohibited after that open parenthesis '('
#17: FILE: trace/events/jbd.h:17:
+		__field(	dev_t,	dev			)

ERROR: space prohibited before that close parenthesis ')'
#17: FILE: trace/events/jbd.h:17:
+		__field(	dev_t,	dev			)

ERROR: space prohibited after that open parenthesis '('
#18: FILE: trace/events/jbd.h:18:
+		__field(	int,	result			)

Re: [PATCH] checkpatch: Find CamelCase definitions of struct/union/enum

[Alexander Holler]

Am 25.09.2013 21:48, schrieb Joe Perches:

> As I've said multiple times, checkpatch is a stupid
> little tool that can be ignored by anyone with
> different tastes.

Just that this isn't true for ordinary innocent people posting patches.
Their patches, if not passed by checkpatch.pl, usally get a full stop at
the first barrier.

Regards,

Alexander Holler

Re: [PATCH] checkpatch: Find CamelCase definitions of struct/union/enum

[Andrew Morton]

On Wed, 25 Sep 2013 21:54:49 +0200 Alexander Holler <holler@ahsoftware.de> wrote:

> Am 25.09.2013 21:48, schrieb Joe Perches:
> 
> > As I've said multiple times, checkpatch is a stupid
> > little tool that can be ignored by anyone with
> > different tastes.
> 
> Just that this isn't true for ordinary innocent people posting patches.

Nobody uses --strict.  Discussion is silly.

Re: [PATCH] checkpatch: Find CamelCase definitions of struct/union/enum

[Al Viro]

On Wed, Sep 25, 2013 at 01:03:52PM -0700, Andrew Morton wrote:
> On Wed, 25 Sep 2013 21:54:49 +0200 Alexander Holler <holler@ahsoftware.de> wrote:
> 
> > Am 25.09.2013 21:48, schrieb Joe Perches:
> > 
> > > As I've said multiple times, checkpatch is a stupid
> > > little tool that can be ignored by anyone with
> > > different tastes.
> > 
> > Just that this isn't true for ordinary innocent people posting patches.
> 
> Nobody uses --strict.  Discussion is silly.

That it is, but you are mistaken about nobody using --strict; I've been
amused a while ago by a patch (duly forwarded to Dave Null for processing,
of course) that "fixed" something like goto Efault, in I don't remember
which file ;-)

Re: [PATCH] checkpatch: Find CamelCase definitions of struct/union/enum

[Theodore Ts'o]

On Wed, Sep 25, 2013 at 01:03:52PM -0700, Andrew Morton wrote:
> 
> Nobody uses --strict.  Discussion is silly.

If "nobody" uses it, why is it there?

   	    	     	    - Ted

Re: [PATCH] checkpatch: Find CamelCase definitions of struct/union/enum

[Andrew Morton]

On Wed, 25 Sep 2013 21:10:49 -0400 "Theodore Ts'o" <tytso@mit.edu> wrote:

> On Wed, Sep 25, 2013 at 01:03:52PM -0700, Andrew Morton wrote:
> > 
> > Nobody uses --strict.  Discussion is silly.
> 
> If "nobody" uses it, why is it there?
> 

Nobody understands hyperbole.

Re: [PATCH] checkpatch: Find CamelCase definitions of struct/union/enum

[Al Viro]

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.

Re: [PATCH] checkpatch: Find CamelCase definitions of struct/union/enum

[Joe Perches]

On Wed, 2013-09-25 at 21:11 +0100, Al Viro wrote:
> 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*

gasp, chuckle, wheeze, smile.

> Most of those are whitespace noise, mostly having nothing whatsoever
> to do with me or my taste.  The rest...  Let's see:
[]
> 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.

A lot of those whitespace noises are the documented preferred
style in CodingStyle.

It's your choice to modify them or not.

Here's what some of those whitespaces elements are that
checkpatch offers to make more CodingStyle conformant via:

$ scripts/checkpatch.pl -f --fix --strict \
	--types=spacing,pointer_location,code_indent,space_before_tab \
	fs/binfmt_misc.c
$ mv fs/binfmt_misc.c.EXPERIMENTAL-checkpatch-fixes fs/binfmt_misc.c

---

 fs/binfmt_misc.c | 68 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index 1c740e1..fc53386 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -108,7 +108,7 @@ static Node *check_file(struct linux_binprm *bprm)
 static int load_misc_binary(struct linux_binprm *bprm)
 {
 	Node *fmt;
-	struct file * interp_file = NULL;
+	struct file *interp_file = NULL;
 	char iname[BINPRM_BUF_SIZE];
 	const char *iname_addr = iname;
 	int retval;
@@ -138,12 +138,12 @@ static int load_misc_binary(struct linux_binprm *bprm)
 		/* if the binary should be opened on behalf of the
 		 * interpreter than keep it open and assign descriptor
 		 * to it */
- 		fd_binary = get_unused_fd();
- 		if (fd_binary < 0) {
- 			retval = fd_binary;
- 			goto _ret;
- 		}
- 		fd_install(fd_binary, bprm->file);
+		fd_binary = get_unused_fd();
+		if (fd_binary < 0) {
+			retval = fd_binary;
+			goto _ret;
+		}
+		fd_install(fd_binary, bprm->file);
 
 		/* if the binary is not readable than enforce mm->dumpable=0
 		   regardless of the interpreter's permissions */
@@ -156,31 +156,31 @@ static int load_misc_binary(struct linux_binprm *bprm)
 		bprm->interp_flags |= BINPRM_FLAGS_EXECFD;
 		bprm->interp_data = fd_binary;
 
- 	} else {
- 		allow_write_access(bprm->file);
- 		fput(bprm->file);
- 		bprm->file = NULL;
- 	}
+	} else {
+		allow_write_access(bprm->file);
+		fput(bprm->file);
+		bprm->file = NULL;
+	}
 	/* make argv[1] be the path to the binary */
-	retval = copy_strings_kernel (1, &bprm->interp, bprm);
+	retval = copy_strings_kernel(1, &bprm->interp, bprm);
 	if (retval < 0)
 		goto _error;
 	bprm->argc++;
 
 	/* add the interp as argv[0] */
-	retval = copy_strings_kernel (1, &iname_addr, bprm);
+	retval = copy_strings_kernel(1, &iname_addr, bprm);
 	if (retval < 0)
 		goto _error;
-	bprm->argc ++;
+	bprm->argc++;
 
 	/* Update interp in case binfmt_script needs it. */
 	retval = bprm_change_interp(iname, bprm);
 	if (retval < 0)
 		goto _error;
 
-	interp_file = open_exec (iname);
-	retval = PTR_ERR (interp_file);
-	if (IS_ERR (interp_file))
+	interp_file = open_exec(iname);
+	retval = PTR_ERR(interp_file);
+	if (IS_ERR(interp_file))
 		goto _error;
 
 	bprm->file = interp_file;
@@ -192,7 +192,7 @@ static int load_misc_binary(struct linux_binprm *bprm)
 		memset(bprm->buf, 0, BINPRM_BUF_SIZE);
 		retval = kernel_read(bprm->file, 0, bprm->buf, BINPRM_BUF_SIZE);
 	} else
-		retval = prepare_binprm (bprm);
+		retval = prepare_binprm(bprm);
 
 	if (retval < 0)
 		goto _error;
@@ -235,9 +235,9 @@ static char *scanarg(char *s, char del)
 	return s;
 }
 
-static char * check_special_flags (char * sfs, Node * e)
+static char *check_special_flags(char *sfs, Node *e)
 {
-	char * p = sfs;
+	char *p = sfs;
 	int cont = 1;
 
 	/* special flags */
@@ -369,7 +369,7 @@ static Node *create_entry(const char __user *buffer, size_t count)
 		goto Einval;
 
 
-	p = check_special_flags (p, e);
+	p = check_special_flags(p, e);
 
 	if (*p == '\n')
 		p++;
@@ -419,7 +419,7 @@ static void entry_status(Node *e, char *page)
 {
 	char *dp;
 	char *status = "disabled";
-	const char * flags = "flags: ";
+	const char *flags = "flags: ";
 
 	if (test_bit(Enabled, &e->flags))
 		status = "enabled";
@@ -433,18 +433,18 @@ static void entry_status(Node *e, char *page)
 	dp = page + strlen(page);
 
 	/* print the special flags */
-	sprintf (dp, "%s", flags);
-	dp += strlen (flags);
+	sprintf(dp, "%s", flags);
+	dp += strlen(flags);
 	if (e->flags & MISC_FMT_PRESERVE_ARGV0) {
-		*dp ++ = 'P';
+		*dp++ = 'P';
 	}
 	if (e->flags & MISC_FMT_OPEN_BINARY) {
-		*dp ++ = 'O';
+		*dp++ = 'O';
 	}
 	if (e->flags & MISC_FMT_CREDENTIALS) {
-		*dp ++ = 'C';
+		*dp++ = 'C';
 	}
-	*dp ++ = '\n';
+	*dp++ = '\n';
 
 
 	if (!test_bit(Magic, &e->flags)) {
@@ -473,7 +473,7 @@ static void entry_status(Node *e, char *page)
 
 static struct inode *bm_get_inode(struct super_block *sb, int mode)
 {
-	struct inode * inode = new_inode(sb);
+	struct inode *inode = new_inode(sb);
 
 	if (inode) {
 		inode->i_ino = get_next_ino();
@@ -513,13 +513,13 @@ static void kill_node(Node *e)
 /* /<entry> */
 
 static ssize_t
-bm_entry_read(struct file * file, char __user * buf, size_t nbytes, loff_t *ppos)
+bm_entry_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
 {
 	Node *e = file_inode(file)->i_private;
 	ssize_t res;
 	char *page;
 
-	if (!(page = (char*) __get_free_page(GFP_KERNEL)))
+	if (!(page = (char *)__get_free_page(GFP_KERNEL)))
 		return -ENOMEM;
 
 	entry_status(e, page);
@@ -639,7 +639,7 @@ bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
 	return simple_read_from_buffer(buf, nbytes, ppos, s, strlen(s));
 }
 
-static ssize_t bm_status_write(struct file * file, const char __user * buffer,
+static ssize_t bm_status_write(struct file *file, const char __user *buffer,
 		size_t count, loff_t *ppos)
 {
 	int res = parse_command(buffer, count);
@@ -674,7 +674,7 @@ static const struct super_operations s_ops = {
 	.evict_inode	= bm_evict_inode,
 };
 
-static int bm_fill_super(struct super_block * sb, void * data, int silent)
+static int bm_fill_super(struct super_block *sb, void *data, int silent)
 {
 	static struct tree_descr bm_files[] = {
 		[2] = {"status", &bm_status_operations, S_IWUSR|S_IRUGO},