checkpatch: kill the bogus camelcase check

checkpatch: kill the bogus camelcase check

[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 <JBottomley@Parallels.com>

---
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index b28cc38..5588dd3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -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",

Re: checkpatch: kill the bogus camelcase check

[Andy Whitcroft]

On Tue, Apr 09, 2013 at 02:50:54PM -0700, James Bottomley wrote:
> 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 <JBottomley@Parallels.com>

Joe, perhaps this could become a strict check?

-apw

Re: checkpatch: kill the bogus camelcase check

[Joe Perches]

On Wed, 2013-04-10 at 14:26 +0100, Andy Whitcroft wrote:
> On Tue, Apr 09, 2013 at 02:50:54PM -0700, James Bottomley wrote:
> > 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 <JBottomley@Parallels.com>
> 
> Joe, perhaps this could become a strict check?

or maybe exclude drivers/scsi and include/scsi/

Re: checkpatch: kill the bogus camelcase check

[Borislav Petkov]

On Wed, Apr 10, 2013 at 07:35:58AM -0700, Joe Perches wrote:
> or maybe exclude drivers/scsi and include/scsi/

and arch/x86/kvm/emulate.c

-- 
Regards/Gruss,
    Boris.

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

Re: checkpatch: kill the bogus camelcase check

[James Bottomley]

On Wed, 2013-04-10 at 16:52 +0200, Borislav Petkov wrote:
> On Wed, Apr 10, 2013 at 07:35:58AM -0700, Joe Perches wrote:
> > 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

Re: checkpatch: kill the bogus camelcase check

[Borislav Petkov]

On Wed, Apr 10, 2013 at 08:07:41AM -0700, James Bottomley wrote:
> On Wed, 2013-04-10 at 16:52 +0200, Borislav Petkov wrote:
> > On Wed, Apr 10, 2013 at 07:35:58AM -0700, Joe Perches wrote:
> > > 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.

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.

-- 
Regards/Gruss,
    Boris.

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

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

[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 <joe@perches.com>
---
 scripts/checkpatch.pl | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3fb6d86..97226fb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -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);
 			}
 		}
 

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

[Borislav Petkov]

On Thu, Apr 11, 2013 at 07:45:03AM -0700, Joe Perches wrote:
> 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 <joe@perches.com>
> ---
>  scripts/checkpatch.pl | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 3fb6d86..97226fb 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -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);

Yep, this is better.

James?

-- 
Regards/Gruss,
    Boris.

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

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

[James Bottomley]

On Wed, 2013-05-01 at 14:34 +0200, Borislav Petkov wrote:
> On Thu, Apr 11, 2013 at 07:45:03AM -0700, Joe Perches wrote:
> > 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 <joe@perches.com>
> > ---
> >  scripts/checkpatch.pl | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 3fb6d86..97226fb 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -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);
> 
> Yep, this is better.

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

Thanks,

James