| Message ID | 20170504193314.4549-1-bill.fischofer@linaro.org |
|---|---|
| State | Accepted |
| Commit | f4386378e466a519d8f97923ba43ea22dec1e933 |
| Headers | show |
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",
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", >
> -----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
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 > > > > > >
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
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 > > > >
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",
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