checkpatch: kill the bogus camelcase check

PATCH In review James Bottomley 4 participants 9 messages Apr 9, 2013 – May 1, 2013

Subsystems (2)

SCSI Checkpatch

lore

Share & tools

lore

James Bottomley

author maintainer

2013-04-09 21:50 UTC #0 lore raw

It's making checkpatch unusable on most drivers because it's spewing tons of bogus warnings. The problem is the assumption that studly caps is always wrong: it isn't if the variables are named after the various conventions in the hardware programming guides (which are usually written by Microsoft people).

In order to encourage people to use checkpatch, it has to be *useful* it can't stray too far into dogmatic things like this that are essentially unfixable by most people who submit patches.

Signed-off-by James Bottomley <[email protected]>

---

scripts/checkpatch.pl +0 ~~-15~~

index b28cc38..5588dd3 100755
@@ -1398,8 +1398,6 @@ sub process {
 	my %suppress_export;
 	my $suppress_statement = 0;
 
-	my %camelcase = ();
-
 	# Pre-scan the patch sanitizing the lines.
 	# Pre-scan the patch looking for any __setup documentation.
 	#

@@ -2925,19 +2923,6 @@ sub process {
 			}
 		}
 
-#CamelCase
-		while ($line =~ m{($Constant|$Lval)}g) {
-			my $var = $1;
-			if ($var !~ /$Constant/ &&
-			    $var =~ /[A-Z]\w*[a-z]|[a-z]\w*[A-Z]/ &&
-			    $var !~ /"^(?:Clear|Set|TestClear|TestSet|)Page[A-Z]/ &&
-			    !defined $camelcase{$var}) {
-				$camelcase{$var} = 1;
-				WARN("CAMELCASE",
-				     "Avoid CamelCase: <$var>\n" . $herecurr);
-			}
-		}
-
 #no spaces allowed after \ in define
 		if ($line=~/\#\s*define.*\\\s$/) {
 			WARN("WHITESPACE_AFTER_LINE_CONTINUATION",

Andy Whitcroft

maintainer

2013-04-10 13:26 UTC #1 lore raw

Quoted 11 lines · James Bottomley

It's making checkpatch unusable on most drivers because it's spewing tons of bogus warnings. The problem is the assumption that studly caps is always wrong: it isn't if the variables are named after the various conventions in the hardware programming guides (which are usually written by Microsoft people).
In order to encourage people to use checkpatch, it has to be *useful* it can't stray too far into dogmatic things like this that are essentially unfixable by most people who submit patches.
Signed-off-by James Bottomley <[email protected]>

Joe, perhaps this could become a strict check?

-apw

Joe Perches

maintainer

2013-04-10 14:35 UTC #2 lore raw

Quoted 7 lines · Andy Whitcroft

Quoted 6 lines · James Bottomley
It's making checkpatch unusable on most drivers because it's spewing tons of bogus warnings. The problem is the assumption that studly caps is always wrong: it isn't if the variables are named after the various conventions in the hardware programming guides (which are usually written by Microsoft people).

Quoted 7 lines

Quoted 5 lines
In order to encourage people to use checkpatch, it has to be *useful* it can't stray too far into dogmatic things like this that are essentially unfixable by most people who submit patches.
Signed-off-by James Bottomley <[email protected]>
Joe, perhaps this could become a strict check?

or maybe exclude drivers/scsi and include/scsi/

Borislav Petkov

2013-04-10 14:52 UTC #3 lore raw

Quoted 1 line · Joe Perches

or maybe exclude drivers/scsi and include/scsi/

and arch/x86/kvm/emulate.c

Signature · 5 lines

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

James Bottomley

author maintainer

2013-04-10 15:07 UTC #4 lore raw

Quoted 4 lines · Borislav Petkov

Quoted 1 line · Joe Perches
or maybe exclude drivers/scsi and include/scsi/
and arch/x86/kvm/emulate.c

Actually, we get this all over drivers. Some of the problems are to do with the fact that the check is wrong, so it thinks things like this

drm_core_has_MTRR

are studly caps when they're not, but we have a lot of device programming manual driven studly caps in PCI, ata, ide etc ..

We also have sanctioned use in mm, things like:

SetPageReserved
ClearPageReserved

But the point still stands. When checkpatch warns about this, there's nothing that the person submitting the patch can do because the usage was already embedded into the file they're patching.

James

Borislav Petkov

2013-04-10 15:19 UTC #5 lore raw

Quoted 14 lines · James Bottomley

Quoted 4 lines · Borislav Petkov
Quoted 1 line · Joe Perches
or maybe exclude drivers/scsi and include/scsi/
and arch/x86/kvm/emulate.c
Actually, we get this all over drivers. Some of the problems are to do with the fact that the check is wrong, so it thinks things like this
drm_core_has_MTRR
are studly caps when they're not, but we have a lot of device programming manual driven studly caps in PCI, ata, ide etc ..

Quoted 8 lines

We also have sanctioned use in mm, things like:
SetPageReserved
ClearPageReserved
But the point still stands. When checkpatch warns about this, there's nothing that the person submitting the patch can do because the usage was already embedded into the file they're patching.

Ha, I hit the nail right on the head! :-) This is exactly the reaction I was aiming at, with mentioning yet another file in the kernel where this check fires.

Yes, excepting certain files is not a good idea. Yes, the check needs to go away because it is plain wrong.

Even if its there, I keep ignoring it which makes checkpatch less useful and trustworthy. So full ACK to the intent to either remove it or make it a suggestion only.

Thanks.

Signature · 5 lines

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Joe Perches

maintainer

2013-04-11 14:45 UTC #6 lore raw

PATCH 1/1 checkpatch: Make camelcase test --strict and less noisy

CamelCase tests are a bit noisy against certain types of code acceptable to some kernel developers.

Make the test applicable only with --strict.

Do not bleat a message on nominally acceptable
CamelCase uses that are separated by an _ like
drm_core_has_MTRR.

Signed-off-by Joe Perches <[email protected]>

---
 scripts/checkpatch.pl | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

scripts/checkpatch.pl +3 -3

index 3fb6d86..97226fb 100755
@@ -2937,12 +2937,12 @@ sub process {
 		while ($line =~ m{($Constant|$Lval)}g) {
 			my $var = $1;
 			if ($var !~ /$Constant/ &&
-			    $var =~ /[A-Z]\w*[a-z]|[a-z]\w*[A-Z]/ &&
+			    $var =~ /[A-Z][a-z]|[a-z][A-Z]/ &&
 			    $var !~ /"^(?:Clear|Set|TestClear|TestSet|)Page[A-Z]/ &&
 			    !defined $camelcase{$var}) {
 				$camelcase{$var} = 1;
-				WARN("CAMELCASE",
-				     "Avoid CamelCase: <$var>\n" . $herecurr);
+				CHK("CAMELCASE",
+				    "Avoid CamelCase: <$var>\n" . $herecurr);
 			}
 		}

Borislav Petkov

2013-05-01 12:34 UTC #7 lore raw

Re: [PATCH] checkpatch: Make camelcase test --strict and less noisy

Quoted 21 lines · Joe Perches

CamelCase tests are a bit noisy against certain types of code acceptable to some kernel developers.
Make the test applicable only with --strict.
Do not bleat a message on nominally acceptable
CamelCase uses that are separated by an _ like
drm_core_has_MTRR.
Signed-off-by Joe Perches <[email protected]>
---
 scripts/checkpatch.pl | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
scripts/checkpatch.pl +0 -0
index 3fb6d86..97226fb 100755
@@ -2937,12 +2937,12 @@ sub process {
 		while ($line =~ m{($Constant|$Lval)}g) {
 			my $var = $1;

scripts/checkpatch.pl sub process { · line 2945

Quoted 10 lines

scripts/checkpatch.pl +3 -3

 			if ($var !~ /$Constant/ &&
-			    $var =~ /[A-Z]\w*[a-z]|[a-z]\w*[A-Z]/ &&
+			    $var =~ /[A-Z][a-z]|[a-z][A-Z]/ &&
 			    $var !~ /"^(?:Clear|Set|TestClear|TestSet|)Page[A-Z]/ &&
 			    !defined $camelcase{$var}) {
 				$camelcase{$var} = 1;
-				WARN("CAMELCASE",
-				     "Avoid CamelCase: <$var>\n" . $herecurr);
+				CHK("CAMELCASE",
+				    "Avoid CamelCase: <$var>\n" . $herecurr);

Yep, this is better.

James?

Signature · 5 lines

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

James Bottomley

author maintainer

2013-05-01 13:50 UTC #8 lore raw

Re: [PATCH] checkpatch: Make camelcase test --strict and less noisy

Quoted 26 lines · Borislav Petkov

Quoted 25 lines · Joe Perches
CamelCase tests are a bit noisy against certain types of code acceptable to some kernel developers.
Make the test applicable only with --strict.
Do not bleat a message on nominally acceptable
CamelCase uses that are separated by an _ like
drm_core_has_MTRR.
Signed-off-by Joe Perches <[email protected]>
---
 scripts/checkpatch.pl | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
scripts/checkpatch.pl +1 -1
index 3fb6d86..97226fb 100755
@@ -2937,12 +2937,12 @@ sub process {
 		while ($line =~ m{($Constant|$Lval)}g) {
 			my $var = $1;
 			if ($var !~ /$Constant/ &&
-			    $var =~ /[A-Z]\w*[a-z]|[a-z]\w*[A-Z]/ &&
+			    $var =~ /[A-Z][a-z]|[a-z][A-Z]/ &&
 			    $var !~ /"^(?:Clear|Set|TestClear|TestSet|)Page[A-Z]/ &&

Quoted 8 lines

scripts/checkpatch.pl sub process { · line 2945

Quoted 6 lines

scripts/checkpatch.pl +2 -2

 			    !defined $camelcase{$var}) {
 				$camelcase{$var} = 1;
-				WARN("CAMELCASE",
-				     "Avoid CamelCase: <$var>\n" . $herecurr);
+				CHK("CAMELCASE",
+				    "Avoid CamelCase: <$var>\n" . $herecurr);

Yep, this is better.

Fine with me ... I don't use --strict so it will make the message deluge go away.

Thanks,

James