[PATCH] checkpatch: Improve CamelCase test for Page

[PATCH] checkpatch: Improve CamelCase test for Page

[Joe Perches]

Add the ClearPage/SetPage/TestClearPage/TestSetPage
variants to the not reported Page CamelCase variables.

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 747bcd7..8a32306 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2930,7 +2930,7 @@ sub process {
 			my $var = $1;
 			if ($var !~ /$Constant/ &&
 			    $var =~ /[A-Z]\w*[a-z]|[a-z]\w*[A-Z]/ &&
-			    $var !~ /^Page[A-Z]/ &&
+			    $var !~ /"^(?:Clear|Set|TestClear|TestSet|)Page[A-Z]/ &&
 			    !defined $camelcase{$var}) {
 				$camelcase{$var} = 1;
 				WARN("CAMELCASE",

Re: [PATCH] checkpatch: Improve CamelCase test for Page

[Peter Hurley]

On Fri, 2013-02-22 at 12:59 -0800, Joe Perches wrote:
> Add the ClearPage/SetPage/TestClearPage/TestSetPage
> variants to the not reported Page CamelCase variables.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  scripts/checkpatch.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 747bcd7..8a32306 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2930,7 +2930,7 @@ sub process {
>  			my $var = $1;
>  			if ($var !~ /$Constant/ &&
>  			    $var =~ /[A-Z]\w*[a-z]|[a-z]\w*[A-Z]/ &&
> -			    $var !~ /^Page[A-Z]/ &&
> +			    $var !~ /"^(?:Clear|Set|TestClear|TestSet|)Page[A-Z]/ &&
>  			    !defined $camelcase{$var}) {
>  				$camelcase{$var} = 1;
>  				WARN("CAMELCASE",
> 

In a recent patch, checkpatch gave this warning.

WARNING: Avoid CamelCase: <tty->SAK_work>
#35: FILE: drivers/tty/tty_io.c:1475:
+	flush_work(&tty->SAK_work);


'SAK' is the acronym for 'Secure Attention Key'. Identifiers which
relate to SAK handling have the above form; eg.,
  do_SAK()
  SAK_work
  do_SAK_work
  fn_SAK
  vc_SAK
  sysrq_handle_SAK

I didn't bother to mention it before, but since your addressing mm
CamelCase exceptions, perhaps

  xxxx_XXXX
  xx_XXX_xxx
  XXX_xxx

could be exceptions as well?

Regards,
Peter Hurley

Re: [PATCH] checkpatch: Improve CamelCase test for Page

[Andrew Morton]

On Fri, 22 Feb 2013 17:01:48 -0500
Peter Hurley <peter@hurleysoftware.com> wrote:

> On Fri, 2013-02-22 at 12:59 -0800, Joe Perches wrote:
> > Add the ClearPage/SetPage/TestClearPage/TestSetPage
> > variants to the not reported Page CamelCase variables.
> > 
> > Signed-off-by: Joe Perches <joe@perches.com>
> > ---
> >  scripts/checkpatch.pl | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 747bcd7..8a32306 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -2930,7 +2930,7 @@ sub process {
> >  			my $var = $1;
> >  			if ($var !~ /$Constant/ &&
> >  			    $var =~ /[A-Z]\w*[a-z]|[a-z]\w*[A-Z]/ &&
> > -			    $var !~ /^Page[A-Z]/ &&
> > +			    $var !~ /"^(?:Clear|Set|TestClear|TestSet|)Page[A-Z]/ &&
> >  			    !defined $camelcase{$var}) {
> >  				$camelcase{$var} = 1;
> >  				WARN("CAMELCASE",
> > 
> 
> In a recent patch, checkpatch gave this warning.
> 
> WARNING: Avoid CamelCase: <tty->SAK_work>
> #35: FILE: drivers/tty/tty_io.c:1475:
> +	flush_work(&tty->SAK_work);

If we start whitelisting these things, it will never end.

My (cruelly spurned) suggestion for this check is to grep the affected
files to see if the symbol is already present and if so, don't warn.

Or just revert the whole thing.  I get tons of camelcase warnings, and
they're always unuseful/incorrect/ignored.

Re: [PATCH] checkpatch: Improve CamelCase test for Page

[Joe Perches]

On Fri, 2013-02-22 at 17:01 -0500, Peter Hurley wrote:
> I didn't bother to mention it before, but since your addressing mm
> CamelCase exceptions, perhaps
> 
>   xxxx_XXXX
>   xx_XXX_xxx
>   XXX_xxx
> 
> could be exceptions as well?

Maybe the check should only be for "[A-Z][a-z]|[a-z][A-Z]"
to make '_' and any non-alpha char a name barrier.

So vars like A_foo or b_9A would be acceptable.

Maybe:

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 747bcd7..e08e9f6 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2929,8 +2929,8 @@ 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 !~ /^Page[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",

Re: [PATCH] checkpatch: Improve CamelCase test for Page

[Peter Hurley]

On Fri, 2013-02-22 at 14:05 -0800, Andrew Morton wrote:
> On Fri, 22 Feb 2013 17:01:48 -0500
> Peter Hurley <peter@hurleysoftware.com> wrote:
> 
> > On Fri, 2013-02-22 at 12:59 -0800, Joe Perches wrote:
> > > Add the ClearPage/SetPage/TestClearPage/TestSetPage
> > > variants to the not reported Page CamelCase variables.
> > > 
> > > Signed-off-by: Joe Perches <joe@perches.com>
> > > ---
> > >  scripts/checkpatch.pl | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > index 747bcd7..8a32306 100755
> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -2930,7 +2930,7 @@ sub process {
> > >  			my $var = $1;
> > >  			if ($var !~ /$Constant/ &&
> > >  			    $var =~ /[A-Z]\w*[a-z]|[a-z]\w*[A-Z]/ &&
> > > -			    $var !~ /^Page[A-Z]/ &&
> > > +			    $var !~ /"^(?:Clear|Set|TestClear|TestSet|)Page[A-Z]/ &&
> > >  			    !defined $camelcase{$var}) {
> > >  				$camelcase{$var} = 1;
> > >  				WARN("CAMELCASE",
> > > 
> > 
> > In a recent patch, checkpatch gave this warning.
> > 
> > WARNING: Avoid CamelCase: <tty->SAK_work>
> > #35: FILE: drivers/tty/tty_io.c:1475:
> > +	flush_work(&tty->SAK_work);
> 
> If we start whitelisting these things, it will never end.
> 
> My (cruelly spurned) suggestion for this check is to grep the affected
> files to see if the symbol is already present and if so, don't warn.
> 
> Or just revert the whole thing.  I get tons of camelcase warnings, and
> they're always unuseful/incorrect/ignored.

Oh. I didn't know the impetus (although I had seen the mm variance and
wondered :)

I don't have an opinion on this. It doesn't bother me, but I'm not my
own maintainer.

Re: [PATCH] checkpatch: Improve CamelCase test for Page

[Peter Hurley]

On Fri, 2013-02-22 at 14:21 -0800, Joe Perches wrote:
> On Fri, 2013-02-22 at 17:01 -0500, Peter Hurley wrote:
> > I didn't bother to mention it before, but since your addressing mm
> > CamelCase exceptions, perhaps
> > 
> >   xxxx_XXXX
> >   xx_XXX_xxx
> >   XXX_xxx
> > 
> > could be exceptions as well?
> 
> Maybe the check should only be for "[A-Z][a-z]|[a-z][A-Z]"
> to make '_' and any non-alpha char a name barrier.
> 
> So vars like A_foo or b_9A would be acceptable.
> 
> Maybe:
> 
> ---
>  scripts/checkpatch.pl | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 747bcd7..e08e9f6 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2929,8 +2929,8 @@ 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 !~ /^Page[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",

Other than that, works for me :)

Re: [PATCH] checkpatch: Improve CamelCase test for Page

[Shuah Khan]

On Fri, Feb 22, 2013 at 3:21 PM, Joe Perches <joe@perches.com> wrote:
> On Fri, 2013-02-22 at 17:01 -0500, Peter Hurley wrote:
>> I didn't bother to mention it before, but since your addressing mm
>> CamelCase exceptions, perhaps
>>
>>   xxxx_XXXX
>>   xx_XXX_xxx
>>   XXX_xxx
>>
>> could be exceptions as well?
>
> Maybe the check should only be for "[A-Z][a-z]|[a-z][A-Z]"
> to make '_' and any non-alpha char a name barrier.
>
> So vars like A_foo or b_9A would be acceptable.
>
> Maybe:
>
> ---
>  scripts/checkpatch.pl | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 747bcd7..e08e9f6 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2929,8 +2929,8 @@ 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 !~ /^Page[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",
>
>

What are the guidelines on camelcase warnings on patches. A recent one
I ran into is on a variable in a structure and fixing it would require
changing the original variable.

WARNING: Avoid CamelCase: <cp->Header.SGList>
#67: FILE: drivers/scsi/hpsa.c:1409:
+		cp->Header.SGList = 0;

WARNING: Avoid CamelCase: <cp->Header.SGTotal>
#68: FILE: drivers/scsi/hpsa.c:1410:
+		cp->Header.SGTotal = 0;

total: 0 errors, 2 warnings, 11 lines checked

One would have to change a large portion of the code to fix it. In
such cases, do we ignore this warning?

-- Shuah

Re: [PATCH] checkpatch: Improve CamelCase test for Page

[Joe Perches]

dOn Fri, 2013-02-22 at 15:57 -0700, Shuah Khan wrote:
> What are the guidelines on camelcase warnings on patches. A recent one
> I ran into is on a variable in a structure and fixing it would require
> changing the original variable.

The same as all other checkpatch warnings.
Ignore the ones you don't agree with.

Errors maybe should be fixed.  You should be able
to ignore those too though.

> One would have to change a large portion of the code to fix it. In
> such cases, do we ignore this warning?

Yes.

Taste is always author's choice.

Of course, lots of things depends on the upstream
path and files you chose to work on.

If you're working in drivers/net, most of these
warnings seem more likely to get patches that have
them rejected.

If you're working on drivers/scsi, it seems you
don't have to bother running checkpatch at all.

Re: [PATCH] checkpatch: Improve CamelCase test for Page

[Andrew Morton]

On Fri, 22 Feb 2013 15:57:16 -0700
Shuah Khan <shuahkhan@gmail.com> wrote:

> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 747bcd7..e08e9f6 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -2929,8 +2929,8 @@ 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 !~ /^Page[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",
> >
> >
> 
> What are the guidelines on camelcase warnings on patches. A recent one
> I ran into is on a variable in a structure and fixing it would require
> changing the original variable.
> 
> WARNING: Avoid CamelCase: <cp->Header.SGList>
> #67: FILE: drivers/scsi/hpsa.c:1409:
> +		cp->Header.SGList = 0;
> 
> WARNING: Avoid CamelCase: <cp->Header.SGTotal>
> #68: FILE: drivers/scsi/hpsa.c:1410:
> +		cp->Header.SGTotal = 0;
> 
> total: 0 errors, 2 warnings, 11 lines checked
> 
> One would have to change a large portion of the code to fix it. In
> such cases, do we ignore this warning?

Yes, ignore the warning.  In my experience this is always the case,
and it happens often.