[PATCHv2] scripts: checkpatch: update to allow additional exceptions

Message ID 20170504193314.4549-1-bill.fischofer@linaro.org
State Accepted
Commit f4386378e466a519d8f97923ba43ea22dec1e933
Headers show

Commit Message

Bill Fischofer May 4, 2017, 7:33 p.m.
Update checkpatch.pl to avoid issuing warnings for use of externs,
volatile, or camelCase.

Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>

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

-- 
2.11.0

Comments

Maxim Uvarov May 8, 2017, 5:08 p.m. | #1
In general I think that this patch is ok. It will be good to fix 
following warning aslo:

WARNING: Missing a blank line after declarations
#39: FILE: platform/linux-generic/include/odp_crypto_internal.h:66:
+               uint32_t bytes;
+               const EVP_MD *evp_md;

total: 0 errors, 1 warnings, 0 checks, 208 lines checked

NOTE: Ignored message types: BIT_MACRO COMPARISON_TO_NULL 
DEPRECATED_VARIABLE NEW_TYPEDEFS SPLIT_STRING SSCANF_TO_KSTRTO

0004-linux-generic-crypto-unify-auth-code.patch has style problems, 
please review.

typedef
odp_crypto_alg_err_t (*crypto_func_t)(odp_crypto_op_param_t *param,
                                       odp_crypto_generic_session_t 
*session);

         struct {
                 uint8_t  key[EVP_MAX_KEY_LENGTH];
                 uint32_t key_length;
                 uint32_t bytes;
                 const EVP_MD *evp_md;
                 crypto_func_t func;
         } auth;



On 05/04/2017 10:33 PM, Bill Fischofer wrote:
> Update checkpatch.pl to avoid issuing warnings for use of externs,

> volatile, or camelCase.

>

> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>

> ---

>   scripts/checkpatch.pl | 6 +++---

>   1 file changed, 3 insertions(+), 3 deletions(-)

>

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl

> index 16316b92..1c27ac60 100755

> --- a/scripts/checkpatch.pl

> +++ b/scripts/checkpatch.pl

> @@ -4273,7 +4273,7 @@ sub process {

>   							$camelcase_file_seeded = 1;

>   						}

>   					}

> -					if (!defined $camelcase{$word}) {

> +					if (!defined $camelcase{$word} && 0) {

>   						$camelcase{$word} = 1;

>   						CHK("CAMELCASE",

>   						    "Avoid CamelCase: <$word>\n" . $herecurr);

> @@ -4620,7 +4620,7 @@ sub process {

>   

>   # no volatiles please

>   		my $asm_volatile = qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b};

> -		if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) {

> +		if ($line =~ /\bvolatile\b/ && 0 && $line !~ /$asm_volatile/) {

>   			WARN("VOLATILE",

>   			     "Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr);

>   		}

> @@ -5134,7 +5134,7 @@ sub process {

>   			if (defined $cond) {

>   				substr($s, 0, length($cond), '');

>   			}

> -			if ($s =~ /^\s*;/ &&

> +			if ($s =~ /^\s*;/ && 0 &&

>   			    $function_name ne 'uninitialized_var')

>   			{

>   				WARN("AVOID_EXTERNS",
Maxim Uvarov May 22, 2017, 7:07 p.m. | #2
Merged!

Maxim.

On 05/04/17 22:33, Bill Fischofer wrote:
> Update checkpatch.pl to avoid issuing warnings for use of externs,

> volatile, or camelCase.

> 

> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>

> ---

>  scripts/checkpatch.pl | 6 +++---

>  1 file changed, 3 insertions(+), 3 deletions(-)

> 

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl

> index 16316b92..1c27ac60 100755

> --- a/scripts/checkpatch.pl

> +++ b/scripts/checkpatch.pl

> @@ -4273,7 +4273,7 @@ sub process {

>  							$camelcase_file_seeded = 1;

>  						}

>  					}

> -					if (!defined $camelcase{$word}) {

> +					if (!defined $camelcase{$word} && 0) {

>  						$camelcase{$word} = 1;

>  						CHK("CAMELCASE",

>  						    "Avoid CamelCase: <$word>\n" . $herecurr);

> @@ -4620,7 +4620,7 @@ sub process {

>  

>  # no volatiles please

>  		my $asm_volatile = qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b};

> -		if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) {

> +		if ($line =~ /\bvolatile\b/ && 0 && $line !~ /$asm_volatile/) {

>  			WARN("VOLATILE",

>  			     "Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr);

>  		}

> @@ -5134,7 +5134,7 @@ sub process {

>  			if (defined $cond) {

>  				substr($s, 0, length($cond), '');

>  			}

> -			if ($s =~ /^\s*;/ &&

> +			if ($s =~ /^\s*;/ && 0 &&

>  			    $function_name ne 'uninitialized_var')

>  			{

>  				WARN("AVOID_EXTERNS",

>
Savolainen, Petri (Nokia - FI/Espoo) May 23, 2017, 12:07 p.m. | #3
> -----Original Message-----

> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Maxim

> Uvarov

> Sent: Monday, May 22, 2017 10:07 PM

> To: lng-odp@lists.linaro.org

> Subject: Re: [lng-odp] [PATCHv2] scripts: checkpatch: update to allow

> additional exceptions

> 

> Merged!

> 

> Maxim.

> 

> On 05/04/17 22:33, Bill Fischofer wrote:

> > Update checkpatch.pl to avoid issuing warnings for use of externs,

> > volatile, or camelCase.

> >

> > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>

> > ---

> >  scripts/checkpatch.pl | 6 +++---

> >  1 file changed, 3 insertions(+), 3 deletions(-)

> >

> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl

> > index 16316b92..1c27ac60 100755

> > --- a/scripts/checkpatch.pl

> > +++ b/scripts/checkpatch.pl

> > @@ -4273,7 +4273,7 @@ sub process {

> >

> 	$camelcase_file_seeded = 1;

> >  						}

> >  					}

> > -					if (!defined

> $camelcase{$word}) {

> > +					if (!defined

> $camelcase{$word} && 0) {



First, I think it's not good to edit the checkpatch.pl itself. We should just use the config file to document what checks are ignored. Also these direct edits are lost when we upgrade to new checkpatch version.

Also, I think camel case check is useful. We are forced to use camel case sometimes due to external lib (openSSL) API, but those are exceptions and should be handled as such. Now this edit opens door for every patch to contain camel case, also when there's no reason to do so. We need reviewers to check for it now, which is a waste.

So, I'd suggest to revert this.



> >

> 	$camelcase{$word} = 1;

> >

> 	CHK("CAMELCASE",

> >

> "Avoid CamelCase: <$word>\n" . $herecurr);

> > @@ -4620,7 +4620,7 @@ sub process {

> >

> >  # no volatiles please

> >  		my $asm_volatile =

> qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b};

> > -		if ($line =~ /\bvolatile\b/ && $line !~

> /$asm_volatile/) {

> > +		if ($line =~ /\bvolatile\b/ && 0 && $line !~

> /$asm_volatile/) {

> >  			WARN("VOLATILE",

> >  			     "Use of volatile is usually wrong:



Why not just add --ignore=VOLATILE into checkpatch.conf ?



> see Documentation/volatile-considered-harmful.txt\n" . $herecurr);

> >  		}

> > @@ -5134,7 +5134,7 @@ sub process {

> >  			if (defined $cond) {

> >  				substr($s, 0, length($cond),

> '');

> >  			}

> > -			if ($s =~ /^\s*;/ &&

> > +			if ($s =~ /^\s*;/ && 0 &&

> >  			    $function_name ne 'uninitialized_var')

> >  			{

> >  				WARN("AVOID_EXTERNS",

> >


Why not just add --ignore= AVOID_EXTERNS into checkpatch.conf ?


It seems that the entire commit should be reverted and config file edited instead.

-Petri
Bill Fischofer May 23, 2017, 12:22 p.m. | #4
On Tue, May 23, 2017 at 7:07 AM, Savolainen, Petri (Nokia - FI/Espoo) <
petri.savolainen@nokia.com> wrote:

>

>

> > -----Original Message-----

> > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of

> Maxim

> > Uvarov

> > Sent: Monday, May 22, 2017 10:07 PM

> > To: lng-odp@lists.linaro.org

> > Subject: Re: [lng-odp] [PATCHv2] scripts: checkpatch: update to allow

> > additional exceptions

> >

> > Merged!

> >

> > Maxim.

> >

> > On 05/04/17 22:33, Bill Fischofer wrote:

> > > Update checkpatch.pl to avoid issuing warnings for use of externs,

> > > volatile, or camelCase.

> > >

> > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>

> > > ---

> > >  scripts/checkpatch.pl | 6 +++---

> > >  1 file changed, 3 insertions(+), 3 deletions(-)

> > >

> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl

> > > index 16316b92..1c27ac60 100755

> > > --- a/scripts/checkpatch.pl

> > > +++ b/scripts/checkpatch.pl

> > > @@ -4273,7 +4273,7 @@ sub process {

> > >

> >       $camelcase_file_seeded = 1;

> > >                                             }

> > >                                     }

> > > -                                   if (!defined

> > $camelcase{$word}) {

> > > +                                   if (!defined

> > $camelcase{$word} && 0) {

>

>

> First, I think it's not good to edit the checkpatch.pl itself. We should

> just use the config file to document what checks are ignored. Also these

> direct edits are lost when we upgrade to new checkpatch version.

>

> Also, I think camel case check is useful. We are forced to use camel case

> sometimes due to external lib (openSSL) API, but those are exceptions and

> should be handled as such. Now this edit opens door for every patch to

> contain camel case, also when there's no reason to do so. We need reviewers

> to check for it now, which is a waste.

>

> So, I'd suggest to revert this.

>

>

The camel case rule we want is very simple: you may use camel case symbols
but you may not define any new ones. I don't believe checkpatch is set up
to support that rule, however. Is there a way of doing this to your
knowledge?


>

>

> > >

> >       $camelcase{$word} = 1;

> > >

> >       CHK("CAMELCASE",

> > >

> > "Avoid CamelCase: <$word>\n" . $herecurr);

> > > @@ -4620,7 +4620,7 @@ sub process {

> > >

> > >  # no volatiles please

> > >             my $asm_volatile =

> > qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b};

> > > -           if ($line =~ /\bvolatile\b/ && $line !~

> > /$asm_volatile/) {

> > > +           if ($line =~ /\bvolatile\b/ && 0 && $line !~

> > /$asm_volatile/) {

> > >                     WARN("VOLATILE",

> > >                          "Use of volatile is usually wrong:

>

>

> Why not just add --ignore=VOLATILE into checkpatch.conf ?

>

>

>

> > see Documentation/volatile-considered-harmful.txt\n" . $herecurr);

> > >             }

> > > @@ -5134,7 +5134,7 @@ sub process {

> > >                     if (defined $cond) {

> > >                             substr($s, 0, length($cond),

> > '');

> > >                     }

> > > -                   if ($s =~ /^\s*;/ &&

> > > +                   if ($s =~ /^\s*;/ && 0 &&

> > >                         $function_name ne 'uninitialized_var')

> > >                     {

> > >                             WARN("AVOID_EXTERNS",

> > >

>

> Why not just add --ignore= AVOID_EXTERNS into checkpatch.conf ?

>

>

> It seems that the entire commit should be reverted and config file edited

> instead.

>


That's a good suggestion.


>

> -Petri

>

>

>

>

>

>
Savolainen, Petri (Nokia - FI/Espoo) May 23, 2017, 12:41 p.m. | #5
From: Bill Fischofer [mailto:bill.fischofer@linaro.org] 

Sent: Tuesday, May 23, 2017 3:22 PM
To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com>
Cc: Maxim Uvarov <maxim.uvarov@linaro.org>; lng-odp@lists.linaro.org
Subject: Re: [lng-odp] [PATCHv2] scripts: checkpatch: update to allow additional exceptions



On Tue, May 23, 2017 at 7:07 AM, Savolainen, Petri (Nokia - FI/Espoo) <mailto:petri.savolainen@nokia.com> wrote:


> -----Original Message-----

> From: lng-odp [mailto:mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Maxim

> Uvarov

> Sent: Monday, May 22, 2017 10:07 PM

> To: mailto:lng-odp@lists.linaro.org

> Subject: Re: [lng-odp] [PATCHv2] scripts: checkpatch: update to allow

> additional exceptions

>

> Merged!

>

> Maxim.

>

> On 05/04/17 22:33, Bill Fischofer wrote:

> > Update http://checkpatch.pl to avoid issuing warnings for use of externs,

> > volatile, or camelCase.

> >

> > Signed-off-by: Bill Fischofer <mailto:bill.fischofer@linaro.org>

> > ---

> >  scripts/http://checkpatch.pl | 6 +++---

> >  1 file changed, 3 insertions(+), 3 deletions(-)

> >

> > diff --git a/scripts/http://checkpatch.pl b/scripts/http://checkpatch.pl

> > index 16316b92..1c27ac60 100755

> > --- a/scripts/http://checkpatch.pl

> > +++ b/scripts/http://checkpatch.pl

> > @@ -4273,7 +4273,7 @@ sub process {

> >

>       $camelcase_file_seeded = 1;

> >                                             }

> >                                     }

> > -                                   if (!defined

> $camelcase{$word}) {

> > +                                   if (!defined

> $camelcase{$word} && 0) {


First, I think it's not good to edit the http://checkpatch.pl itself. We should just use the config file to document what checks are ignored. Also these direct edits are lost when we upgrade to new checkpatch version.

Also, I think camel case check is useful. We are forced to use camel case sometimes due to external lib (openSSL) API, but those are exceptions and should be handled as such. Now this edit opens door for every patch to contain camel case, also when there's no reason to do so. We need reviewers to check for it now, which is a waste.

So, I'd suggest to revert this.

The camel case rule we want is very simple: you may use camel case symbols but you may not define any new ones. I don't believe checkpatch is set up to support that rule, however. Is there a way of doing this to your knowledge? 
 
[petri]
Just the way we have done it so far: checkpatch warns on all camel cases and reviewers pass only those that have legitimate reasoning (== external lib usage) .

-Petri
Maxim Uvarov May 23, 2017, 8:22 p.m. | #6
Ok, I will test with --ignore options. If it works than might be better
to use it.

Maxim.

On 05/23/17 15:41, Savolainen, Petri (Nokia - FI/Espoo) wrote:
> 

> 

> From: Bill Fischofer [mailto:bill.fischofer@linaro.org] 

> Sent: Tuesday, May 23, 2017 3:22 PM

> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com>

> Cc: Maxim Uvarov <maxim.uvarov@linaro.org>; lng-odp@lists.linaro.org

> Subject: Re: [lng-odp] [PATCHv2] scripts: checkpatch: update to allow additional exceptions

> 

> 

> 

> On Tue, May 23, 2017 at 7:07 AM, Savolainen, Petri (Nokia - FI/Espoo) <mailto:petri.savolainen@nokia.com> wrote:

> 

> 

>> -----Original Message-----

>> From: lng-odp [mailto:mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Maxim

>> Uvarov

>> Sent: Monday, May 22, 2017 10:07 PM

>> To: mailto:lng-odp@lists.linaro.org

>> Subject: Re: [lng-odp] [PATCHv2] scripts: checkpatch: update to allow

>> additional exceptions

>>

>> Merged!

>>

>> Maxim.

>>

>> On 05/04/17 22:33, Bill Fischofer wrote:

>>> Update http://checkpatch.pl to avoid issuing warnings for use of externs,

>>> volatile, or camelCase.

>>>

>>> Signed-off-by: Bill Fischofer <mailto:bill.fischofer@linaro.org>

>>> ---

>>>   scripts/http://checkpatch.pl | 6 +++---

>>>   1 file changed, 3 insertions(+), 3 deletions(-)

>>>

>>> diff --git a/scripts/http://checkpatch.pl b/scripts/http://checkpatch.pl

>>> index 16316b92..1c27ac60 100755

>>> --- a/scripts/http://checkpatch.pl

>>> +++ b/scripts/http://checkpatch.pl

>>> @@ -4273,7 +4273,7 @@ sub process {

>>>

>>        $camelcase_file_seeded = 1;

>>>                                              }

>>>                                      }

>>> -                                   if (!defined

>> $camelcase{$word}) {

>>> +                                   if (!defined

>> $camelcase{$word} && 0) {

> 

> First, I think it's not good to edit the http://checkpatch.pl itself. We should just use the config file to document what checks are ignored. Also these direct edits are lost when we upgrade to new checkpatch version.

> 

> Also, I think camel case check is useful. We are forced to use camel case sometimes due to external lib (openSSL) API, but those are exceptions and should be handled as such. Now this edit opens door for every patch to contain camel case, also when there's no reason to do so. We need reviewers to check for it now, which is a waste.

> 

> So, I'd suggest to revert this.

> 

> The camel case rule we want is very simple: you may use camel case symbols but you may not define any new ones. I don't believe checkpatch is set up to support that rule, however. Is there a way of doing this to your knowledge? 

>  

> [petri]

> Just the way we have done it so far: checkpatch warns on all camel cases and reviewers pass only those that have legitimate reasoning (== external lib usage) .

> 

> -Petri

> 

> 

> 

>

Patch hide | download patch | download mbox

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 16316b92..1c27ac60 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4273,7 +4273,7 @@  sub process {
 							$camelcase_file_seeded = 1;
 						}
 					}
-					if (!defined $camelcase{$word}) {
+					if (!defined $camelcase{$word} && 0) {
 						$camelcase{$word} = 1;
 						CHK("CAMELCASE",
 						    "Avoid CamelCase: <$word>\n" . $herecurr);
@@ -4620,7 +4620,7 @@  sub process {
 
 # no volatiles please
 		my $asm_volatile = qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b};
-		if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) {
+		if ($line =~ /\bvolatile\b/ && 0 && $line !~ /$asm_volatile/) {
 			WARN("VOLATILE",
 			     "Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr);
 		}
@@ -5134,7 +5134,7 @@  sub process {
 			if (defined $cond) {
 				substr($s, 0, length($cond), '');
 			}
-			if ($s =~ /^\s*;/ &&
+			if ($s =~ /^\s*;/ && 0 &&
 			    $function_name ne 'uninitialized_var')
 			{
 				WARN("AVOID_EXTERNS",