All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH v1] checkpatch: Handle FILE pointer type
@ 2022-09-01 14:59 Mickaël Salaün
  2022-09-01 15:49 ` Joe Perches
  0 siblings, 1 reply; 7+ messages in thread
From: Mickaël Salaün @ 2022-09-01 14:59 UTC (permalink / raw)
  To: Andy Whitcroft, Joe Perches
  Cc: Mickaël Salaün, Dwaipayan Ray, Lukas Bulwahn,
	Shuah Khan, linux-kernel, linux-kselftest

When using a "FILE *" type, checkpatch considers this an error.  Fix
this by explicitly defining "FILE" as a common type.

This is useful for user space patches.

With this patch, we now get:
 static void a(FILE *const b)

 <E> <E> <_>WS( )
 <E> <E> <_>IDENT(static)
 <E> <V> <_>WS( )
 <E> <V> <_>DECLARE(void )
 <E> <T> <_>FUNC(a)
 <E> <V> <V>PAREN('(')
 <EV> <N> <_>DECLARE(FILE *const )
 <EV> <T> <_>IDENT(b)
 <EV> <V> <_>PAREN(')') -> V
 <E> <V> <_>WS(
)
32 > . static void a(FILE *const b)
32 > EEVVVVVVVTTTTTVNTTTTTTTTTTTTVVV
32 >  ______________________________

Another error may be throw when we use FIXTURE_{DATA,VARIANT}() structs,
as defined in kselftest_harness.h .  The commented
$typeKselftestHarnessTypedefs should fix it but such definition is
considered as a function instead, because of the closing parenthesis.
I'd like some help to fix this as well.

With $typeKselftestHarnessTypedefs added to $typeTypedefs, we get:
 static void c(const FIXTURE_VARIANT(d) *const e)

 <E> <E> <_>WS( )
 <E> <E> <_>IDENT(static)
 <E> <V> <_>WS( )
 <E> <V> <_>DECLARE(void )
 <E> <T> <_>FUNC(c)
 <E> <V> <V>PAREN('(')
 <EV> <N> <_>MODIFIER(const)
 <EV> <T> <_>WS( )
 <EV> <T> <_>FUNC(FIXTURE_VARIANT)
 <EV> <V> <V>PAREN('(')
 <EVV> <N> <_>IDENT(d)
 <EVV> <V> <_>PAREN(')') -> V
 <EV> <V> <_>WS( )
 <EV> <V> <_>OPV(*)
 <EV> <N> <_>MODIFIER(const)
 <EV> <T> <_>WS( )
 <EV> <T> <_>IDENT(e)
 <EV> <V> <_>PAREN(')') -> V
 <E> <V> <_>WS(
)
30 > . static void c(const FIXTURE_VARIANT(d) *const e)
30 > EEVVVVVVVTTTTTVNTTTTTTVVVVVVVVVVVVVVVNVVVNTTTTTTVVV
30 >  ________________________________________B_________

Cc: Andy Whitcroft <apw@canonical.com>
Cc: Dwaipayan Ray <dwaipayanray1@gmail.com>
Cc: Joe Perches <joe@perches.com>
Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20220901145948.1456353-1-mic@digikod.net
---
 scripts/checkpatch.pl | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 79e759aac543..016b47c35742 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -576,10 +576,17 @@ our $typeKernelTypedefs = qr{(?x:
 	(?:__)?(?:u|s|be|le)(?:8|16|32|64)|
 	atomic_t
 )};
+our $typeStdioTypedefs = qr{(?x:
+	FILE
+)};
+# our $typeKselftestHarnessTypedefs = qr{(?x:
+# 	FIXTURE_(?:DATA|VARIANT)\($Ident\)
+# )};
 our $typeTypedefs = qr{(?x:
 	$typeC99Typedefs\b|
 	$typeOtherOSTypedefs\b|
-	$typeKernelTypedefs\b
+	$typeKernelTypedefs\b|
+	$typeStdioTypedefs\b
 )};
 
 our $zero_initializer = qr{(?:(?:0[xX])?0+$Int_type?|NULL|false)\b};

base-commit: b90cb1053190353cc30f0fef0ef1f378ccc063c5
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH v1] checkpatch: Handle FILE pointer type
  2022-09-01 14:59 [RFC PATCH v1] checkpatch: Handle FILE pointer type Mickaël Salaün
@ 2022-09-01 15:49 ` Joe Perches
  2022-09-01 15:53   ` Mickaël Salaün
  2022-09-01 18:22   ` Joe Perches
  0 siblings, 2 replies; 7+ messages in thread
From: Joe Perches @ 2022-09-01 15:49 UTC (permalink / raw)
  To: Mickaël Salaün, Andy Whitcroft
  Cc: Dwaipayan Ray, Lukas Bulwahn, Shuah Khan, linux-kernel, linux-kselftest

On Thu, 2022-09-01 at 16:59 +0200, Mickaël Salaün wrote:
> When using a "FILE *" type, checkpatch considers this an error.  Fix
> this by explicitly defining "FILE" as a common type.
[]
> Another error may be throw when we use FIXTURE_{DATA,VARIANT}() structs,
> as defined in kselftest_harness.h .
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -576,10 +576,17 @@ our $typeKernelTypedefs = qr{(?x:
>  	(?:__)?(?:u|s|be|le)(?:8|16|32|64)|
>  	atomic_t
>  )};
> +our $typeStdioTypedefs = qr{(?x:
> +	FILE
> +)};

I'm fine with this.

> +# our $typeKselftestHarnessTypedefs = qr{(?x:
> +# 	FIXTURE_(?:DATA|VARIANT)\($Ident\)
> +# )};

But not this.  Random userspace typedefs should likely
be kept in some local version of checkpatch.

Or maybe add a command line option like --additional_typedefs=<file>.

>  our $typeTypedefs = qr{(?x:
>  	$typeC99Typedefs\b|
>  	$typeOtherOSTypedefs\b|
> -	$typeKernelTypedefs\b
> +	$typeKernelTypedefs\b|
> +	$typeStdioTypedefs\b
>  )};


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH v1] checkpatch: Handle FILE pointer type
  2022-09-01 15:49 ` Joe Perches
@ 2022-09-01 15:53   ` Mickaël Salaün
  2022-09-01 18:22   ` Joe Perches
  1 sibling, 0 replies; 7+ messages in thread
From: Mickaël Salaün @ 2022-09-01 15:53 UTC (permalink / raw)
  To: Joe Perches, Andy Whitcroft
  Cc: Dwaipayan Ray, Lukas Bulwahn, Shuah Khan, linux-kernel, linux-kselftest



On 01/09/2022 17:49, Joe Perches wrote:
> On Thu, 2022-09-01 at 16:59 +0200, Mickaël Salaün wrote:

[...]

>> +# our $typeKselftestHarnessTypedefs = qr{(?x:
>> +# 	FIXTURE_(?:DATA|VARIANT)\($Ident\)
>> +# )};
> 
> But not this.  Random userspace typedefs should likely
> be kept in some local version of checkpatch.
> 
> Or maybe add a command line option like --additional_typedefs=<file>.

This is part of the kselftest harness API. Could we take this into 
account for files under tools/testing/selftests ?

> 
>>   our $typeTypedefs = qr{(?x:
>>   	$typeC99Typedefs\b|
>>   	$typeOtherOSTypedefs\b|
>> -	$typeKernelTypedefs\b
>> +	$typeKernelTypedefs\b|
>> +	$typeStdioTypedefs\b
>>   )};
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH v1] checkpatch: Handle FILE pointer type
  2022-09-01 15:49 ` Joe Perches
  2022-09-01 15:53   ` Mickaël Salaün
@ 2022-09-01 18:22   ` Joe Perches
  2022-09-02  9:04     ` Mickaël Salaün
  1 sibling, 1 reply; 7+ messages in thread
From: Joe Perches @ 2022-09-01 18:22 UTC (permalink / raw)
  To: Mickaël Salaün, Andy Whitcroft
  Cc: Dwaipayan Ray, Lukas Bulwahn, Shuah Khan, linux-kernel,
	linux-kselftest, Jerome Forissier

On Thu, 2022-09-01 at 11:49 -0400, Joe Perches wrote:
> On Thu, 2022-09-01 at 16:59 +0200, Mickaël Salaün wrote:
> > When using a "FILE *" type, checkpatch considers this an error.  Fix
> > this by explicitly defining "FILE" as a common type.
> []
> > Another error may be throw when we use FIXTURE_{DATA,VARIANT}() structs,
> > as defined in kselftest_harness.h .
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -576,10 +576,17 @@ our $typeKernelTypedefs = qr{(?x:
> >  	(?:__)?(?:u|s|be|le)(?:8|16|32|64)|
> >  	atomic_t
> >  )};
> > +our $typeStdioTypedefs = qr{(?x:
> > +	FILE
> > +)};
> 
> I'm fine with this.
> 
> > +# our $typeKselftestHarnessTypedefs = qr{(?x:
> > +# 	FIXTURE_(?:DATA|VARIANT)\($Ident\)
> > +# )};
> 
> But not this.  Random userspace typedefs should likely
> be kept in some local version of checkpatch.
> 
> Or maybe add a command line option like --additional_typedefs=<file>.

Oops.  I forgot it already exists:

  --typedefsfile             Read additional types from this file

commit 75ad8c575a5ad105e2afc2051c68abceb9c65431
Author: Jerome Forissier <jerome.forissier@linaro.org>
Date:   Mon May 8 15:56:00 2017 -0700

    checkpatch: add --typedefsfile
    
    When using checkpatch on out-of-tree code, it may occur that some
    project-specific types are used, which will cause spurious warnings.
    Add the --typedefsfile option as a way to extend the known types and
    deal with this issue.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH v1] checkpatch: Handle FILE pointer type
  2022-09-01 18:22   ` Joe Perches
@ 2022-09-02  9:04     ` Mickaël Salaün
  2022-09-02 10:39       ` Joe Perches
  0 siblings, 1 reply; 7+ messages in thread
From: Mickaël Salaün @ 2022-09-02  9:04 UTC (permalink / raw)
  To: Joe Perches, Andy Whitcroft
  Cc: Dwaipayan Ray, Lukas Bulwahn, Shuah Khan, linux-kernel,
	linux-kselftest, Jerome Forissier



On 01/09/2022 20:22, Joe Perches wrote:
> On Thu, 2022-09-01 at 11:49 -0400, Joe Perches wrote:
>> On Thu, 2022-09-01 at 16:59 +0200, Mickaël Salaün wrote:
>>> When using a "FILE *" type, checkpatch considers this an error.  Fix
>>> this by explicitly defining "FILE" as a common type.
>> []
>>> Another error may be throw when we use FIXTURE_{DATA,VARIANT}() structs,
>>> as defined in kselftest_harness.h .
>> []
>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> []
>>> @@ -576,10 +576,17 @@ our $typeKernelTypedefs = qr{(?x:
>>>   	(?:__)?(?:u|s|be|le)(?:8|16|32|64)|
>>>   	atomic_t
>>>   )};
>>> +our $typeStdioTypedefs = qr{(?x:
>>> +	FILE
>>> +)};
>>
>> I'm fine with this.
>>
>>> +# our $typeKselftestHarnessTypedefs = qr{(?x:
>>> +# 	FIXTURE_(?:DATA|VARIANT)\($Ident\)
>>> +# )};
>>
>> But not this.  Random userspace typedefs should likely
>> be kept in some local version of checkpatch.
>>
>> Or maybe add a command line option like --additional_typedefs=<file>.
> 
> Oops.  I forgot it already exists:
> 
>    --typedefsfile             Read additional types from this file
> 
> commit 75ad8c575a5ad105e2afc2051c68abceb9c65431
> Author: Jerome Forissier <jerome.forissier@linaro.org>
> Date:   Mon May 8 15:56:00 2017 -0700
> 
>      checkpatch: add --typedefsfile
>      
>      When using checkpatch on out-of-tree code, it may occur that some
>      project-specific types are used, which will cause spurious warnings.
>      Add the --typedefsfile option as a way to extend the known types and
>      deal with this issue.
> 

This doesn't work for the FIXTURE_DATA() case. And I'm not sure how 
contributors would know that they need to use this option with a 
specific file.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH v1] checkpatch: Handle FILE pointer type
  2022-09-02  9:04     ` Mickaël Salaün
@ 2022-09-02 10:39       ` Joe Perches
  2022-09-02 11:04         ` Mickaël Salaün
  0 siblings, 1 reply; 7+ messages in thread
From: Joe Perches @ 2022-09-02 10:39 UTC (permalink / raw)
  To: Mickaël Salaün, Andy Whitcroft
  Cc: Dwaipayan Ray, Lukas Bulwahn, Shuah Khan, linux-kernel,
	linux-kselftest, Jerome Forissier

On Fri, 2022-09-02 at 11:04 +0200, Mickaël Salaün wrote:
> On 01/09/2022 20:22, Joe Perches wrote:
> > On Thu, 2022-09-01 at 11:49 -0400, Joe Perches wrote:
> > > On Thu, 2022-09-01 at 16:59 +0200, Mickaël Salaün wrote:
> > > > When using a "FILE *" type, checkpatch considers this an error.  Fix
> > > > this by explicitly defining "FILE" as a common type.
> > > []
> > > > Another error may be throw when we use FIXTURE_{DATA,VARIANT}() structs,
> > > > as defined in kselftest_harness.h .
> > > []
> > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > []
> > > > @@ -576,10 +576,17 @@ our $typeKernelTypedefs = qr{(?x:
> > > >   	(?:__)?(?:u|s|be|le)(?:8|16|32|64)|
> > > >   	atomic_t
> > > >   )};
> > > > +our $typeStdioTypedefs = qr{(?x:
> > > > +	FILE
> > > > +)};
> > > 
> > > I'm fine with this.
> > > 
> > > > +# our $typeKselftestHarnessTypedefs = qr{(?x:
> > > > +# 	FIXTURE_(?:DATA|VARIANT)\($Ident\)
> > > > +# )};
> > > 
> > > But not this.  Random userspace typedefs should likely
> > > be kept in some local version of checkpatch.
> > > 
> > > Or maybe add a command line option like --additional_typedefs=<file>.
> > 
> > Oops.  I forgot it already exists:
> > 
> >    --typedefsfile             Read additional types from this file
> > 
> > commit 75ad8c575a5ad105e2afc2051c68abceb9c65431
> > Author: Jerome Forissier <jerome.forissier@linaro.org>
> > Date:   Mon May 8 15:56:00 2017 -0700
> > 
> >      checkpatch: add --typedefsfile
> >      
> >      When using checkpatch on out-of-tree code, it may occur that some
> >      project-specific types are used, which will cause spurious warnings.
> >      Add the --typedefsfile option as a way to extend the known types and
> >      deal with this issue.
> 
> This doesn't work for the FIXTURE_DATA() case.

checkpatch is a stupid little script.
It's not a c preprocessor nor a syntax complete compiler.
It's really easy for macros to make its output invalid.
If you feed obfuscated c to checkpatch, it'd be confused.
(Same is true for tools like coccinelle btw, though cocci is far better)
checkpatch will never be comprehensive nor perfect.
It's expected its users will use their common sense about its
output message validity.

> And I'm not sure how 
> contributors would know that they need to use this option with a 
> specific file.

--help exists

Maybe Documentation/dev-tools/checkpatch.rst could be expanded for
--verbose mode.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH v1] checkpatch: Handle FILE pointer type
  2022-09-02 10:39       ` Joe Perches
@ 2022-09-02 11:04         ` Mickaël Salaün
  0 siblings, 0 replies; 7+ messages in thread
From: Mickaël Salaün @ 2022-09-02 11:04 UTC (permalink / raw)
  To: Joe Perches, Andy Whitcroft
  Cc: Dwaipayan Ray, Lukas Bulwahn, Shuah Khan, linux-kernel,
	linux-kselftest, Jerome Forissier


On 02/09/2022 12:39, Joe Perches wrote:
> On Fri, 2022-09-02 at 11:04 +0200, Mickaël Salaün wrote:
>> On 01/09/2022 20:22, Joe Perches wrote:
>>> On Thu, 2022-09-01 at 11:49 -0400, Joe Perches wrote:
>>>> On Thu, 2022-09-01 at 16:59 +0200, Mickaël Salaün wrote:
>>>>> When using a "FILE *" type, checkpatch considers this an error.  Fix
>>>>> this by explicitly defining "FILE" as a common type.
>>>> []
>>>>> Another error may be throw when we use FIXTURE_{DATA,VARIANT}() structs,
>>>>> as defined in kselftest_harness.h .
>>>> []
>>>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>>> []
>>>>> @@ -576,10 +576,17 @@ our $typeKernelTypedefs = qr{(?x:
>>>>>    	(?:__)?(?:u|s|be|le)(?:8|16|32|64)|
>>>>>    	atomic_t
>>>>>    )};
>>>>> +our $typeStdioTypedefs = qr{(?x:
>>>>> +	FILE
>>>>> +)};
>>>>
>>>> I'm fine with this.
>>>>
>>>>> +# our $typeKselftestHarnessTypedefs = qr{(?x:
>>>>> +# 	FIXTURE_(?:DATA|VARIANT)\($Ident\)
>>>>> +# )};
>>>>
>>>> But not this.  Random userspace typedefs should likely
>>>> be kept in some local version of checkpatch.
>>>>
>>>> Or maybe add a command line option like --additional_typedefs=<file>.
>>>
>>> Oops.  I forgot it already exists:
>>>
>>>     --typedefsfile             Read additional types from this file
>>>
>>> commit 75ad8c575a5ad105e2afc2051c68abceb9c65431
>>> Author: Jerome Forissier <jerome.forissier@linaro.org>
>>> Date:   Mon May 8 15:56:00 2017 -0700
>>>
>>>       checkpatch: add --typedefsfile
>>>       
>>>       When using checkpatch on out-of-tree code, it may occur that some
>>>       project-specific types are used, which will cause spurious warnings.
>>>       Add the --typedefsfile option as a way to extend the known types and
>>>       deal with this issue.
>>
>> This doesn't work for the FIXTURE_DATA() case.
> 
> checkpatch is a stupid little script.
> It's not a c preprocessor nor a syntax complete compiler.
> It's really easy for macros to make its output invalid.
> If you feed obfuscated c to checkpatch, it'd be confused.
> (Same is true for tools like coccinelle btw, though cocci is far better)
> checkpatch will never be comprehensive nor perfect.
> It's expected its users will use their common sense about its
> output message validity.
> 
>> And I'm not sure how
>> contributors would know that they need to use this option with a
>> specific file.
> 
> --help exists
> 
> Maybe Documentation/dev-tools/checkpatch.rst could be expanded for
> --verbose mode.

I was thinking about which file to use, but I understand your point. 
I'll send a v2 with only the "FILE" addition.
FIXTURE_{DATA,VARIANT}() will just not be handled but that's OK.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-09-02 11:04 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-01 14:59 [RFC PATCH v1] checkpatch: Handle FILE pointer type Mickaël Salaün
2022-09-01 15:49 ` Joe Perches
2022-09-01 15:53   ` Mickaël Salaün
2022-09-01 18:22   ` Joe Perches
2022-09-02  9:04     ` Mickaël Salaün
2022-09-02 10:39       ` Joe Perches
2022-09-02 11:04         ` Mickaël Salaün

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.