Re: checkpatch false positive

Re: checkpatch false positive

[Joe Perches]

On Tue, 2014-10-21 at 10:14 +0200, Hans de Goede wrote:
> Hi,
> 
> Checkpatch gives the following warning:
> 
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #31:
> new file mode 100644
> 
> total: 0 errors, 1 warnings, 352 lines checked
> 
> 0001-input-Add-new-sun4i-lradc-keys-driver.patch has style problems, please review.
> 
> If any of these errors are false positives, please report
> them to the maintainer, see CHECKPATCH in MAINTAINERS.
> 
> On a patch of mine, even though it updates MAINTAINERS properly, it would
> be nice if checkpatch would check for a hunk updating MAINTAINERS, and then
> would not issue this warning (note my perl-foo is way too weak to fix this
> myself).
> 
> I've attached the patch triggering the warning.

Hi Hans.

It's not really fixable.  Of course you are welcome to
try though.

Many patches are discrete and the entire series isn't
visible to a single MAINTAINERS update scan by checkpatch.

Re: checkpatch false positive

[Hans de Goede]

Hi,

On 10/21/2014 10:28 AM, Joe Perches wrote:
> On Tue, 2014-10-21 at 10:14 +0200, Hans de Goede wrote:
>> Hi,
>>
>> Checkpatch gives the following warning:
>>
>> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
>> #31:
>> new file mode 100644
>>
>> total: 0 errors, 1 warnings, 352 lines checked
>>
>> 0001-input-Add-new-sun4i-lradc-keys-driver.patch has style problems, please review.
>>
>> If any of these errors are false positives, please report
>> them to the maintainer, see CHECKPATCH in MAINTAINERS.
>>
>> On a patch of mine, even though it updates MAINTAINERS properly, it would
>> be nice if checkpatch would check for a hunk updating MAINTAINERS, and then
>> would not issue this warning (note my perl-foo is way too weak to fix this
>> myself).
>>
>> I've attached the patch triggering the warning.
> 
> Hi Hans.
> 
> It's not really fixable.  Of course you are welcome to
> try though.
> 
> Many patches are discrete and the entire series isn't
> visible to a single MAINTAINERS update scan by checkpatch.

I understand that in that scenario it is not fixable, but in the case I was
talking about the addition of the new file and the MAINTAINERS update
are part of a single patch (adding a small new driver), it would be nice
if at least in that case checkpatch would not complain. This does not even
need to be really smart, it could just check if a patch introducing new
files is touching MAINTAINERS at all, and if it does suppress the warning.

As said my perl-foo is weak, so I will not be trying to fix this myself.

Regards,

Hans

Checkpatch: False positive

[Viresh Kumar]

Hi Andy/Joe,

I got a warning today for my cover-letter, and it looked like a false
positive. Please have a look, based of v4.2-rc2.

-----------------------
0000-cover-letter.patch
-----------------------
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#31: 
 arch/x86/kernel/hpet.c         | 198 ++++++++++++++++++++++++++---------------

-- 
viresh

Re: Checkpatch: False positive

[Joe Perches]

On Thu, 2015-07-16 at 16:25 +0530, Viresh Kumar wrote:
> Hi Andy/Joe,
> 
> I got a warning today for my cover-letter, and it looked like a false
> positive. Please have a look, based of v4.2-rc2.
> -----------------------
> 0000-cover-letter.patch
> -----------------------
> WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
> #31: 
>  arch/x86/kernel/hpet.c         | 198 ++++++++++++++++++++++++++---------------

There are a lot of other false positives for this test.

Maybe it's not worth checking for this condition and the
test should be removed as "fixing" it may not be feasible.

Re: Checkpatch: False positive

[Andy Whitcroft]

On Thu, Jul 16, 2015 at 08:35:58AM -0700, Joe Perches wrote:
> > #31: 
> >  arch/x86/kernel/hpet.c         | 198 ++++++++++++++++++++++++++---------------

I guess those are in the limbo land between the end of message and
beginning of the patch itself.  Perhaps the test should at least stop at
the end of header marker, at the '---'.

-apw

Re: Checkpatch: False positive

[Joe Perches]

On Thu, 2015-07-16 at 16:43 +0100, Andy Whitcroft wrote:
> On Thu, Jul 16, 2015 at 08:35:58AM -0700, Joe Perches wrote:
> > > #31: 
> > >  arch/x86/kernel/hpet.c         | 198 ++++++++++++++++++++++++++---------------
> 
> I guess those are in the limbo land between the end of message and
> beginning of the patch itself.  Perhaps the test should at least stop at
> the end of header marker, at the '---'.
> 
> -apw

Maybe, but the test already stops at signatures like
Signed-off-by: that should always be above the ---.

This might help, but there are _many_ false positives.

The other thing that might help is for people to take
the warnings the script produces less seriously.

Maybe convert:

ERROR -> defect
WARNING -> unstylish
CHECK -> nitpick

or some such

---

 scripts/checkpatch.pl | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d5ce29a..5e7afa7 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2213,6 +2213,11 @@ sub process {
 			$in_commit_log = 0;
 		}
 
+# Check for patch separator
+		if ($line =~ /^---$/) {
+			$in_commit_log = 0;
+		}
+
 # Check if MAINTAINERS is being updated.  If so, there's probably no need to
 # emit the "does MAINTAINERS need updating?" message on file add/move/delete
 		if ($line =~ /^\s*MAINTAINERS\s*\|/) {

Re: Checkpatch: False positive

[Andy Whitcroft]

On Thu, Jul 16, 2015 at 08:58:56AM -0700, Joe Perches wrote:
> On Thu, 2015-07-16 at 16:43 +0100, Andy Whitcroft wrote:
> > On Thu, Jul 16, 2015 at 08:35:58AM -0700, Joe Perches wrote:
> > > > #31: 
> > > >  arch/x86/kernel/hpet.c         | 198 ++++++++++++++++++++++++++---------------
> > 
> > I guess those are in the limbo land between the end of message and
> > beginning of the patch itself.  Perhaps the test should at least stop at
> > the end of header marker, at the '---'.
> > 
> > -apw
> 
> Maybe, but the test already stops at signatures like
> Signed-off-by: that should always be above the ---.
> 
> This might help, but there are _many_ false positives.
> 
> The other thing that might help is for people to take
> the warnings the script produces less seriously.
> 
> Maybe convert:
> 
> ERROR -> defect
> WARNING -> unstylish
> CHECK -> nitpick

Heh, that has long been the main issue, please please believe your brain
not checkpatch.  But yes some less inflamitory words might, just might,
reduce the noise.

-apw

checkpatch false positive.

[Dave Jones]

I just got this from a patch I merged..

ERROR: need consistent spacing around '*' (ctx:WxV)
#121: FILE: arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c:113:
+static struct pcc_cpu __percpu *pcc_cpu_info;
                                ^
which doesn't seem right.

	Dave

checkpatch false positive

[Richard Kennedy]

I'm getting this error from checkpatch which is a false positive AFAICT.
I don't see any other way to code this macro so maybe this rule
shouldn't apply?.

	ERROR: space prohibited before open square bracket '['
	#24: FILE: drivers/staging/wlan-ng/p80211wext.c:1685:
	+#define IW_IOCTL(x) [(x)-SIOCSIWCOMMIT]

regards
Richard

Re: checkpatch false positive

[Andy Whitcroft]

On Wed, Mar 17, 2010 at 11:00:29AM +0000, Richard Kennedy wrote:
> I'm getting this error from checkpatch which is a false positive AFAICT.
> I don't see any other way to code this macro so maybe this rule
> shouldn't apply?.
> 
> 	ERROR: space prohibited before open square bracket '['
> 	#24: FILE: drivers/staging/wlan-ng/p80211wext.c:1685:
> 	+#define IW_IOCTL(x) [(x)-SIOCSIWCOMMIT]

Yes that is definatly false, feed free to ignore it.  I'll have a look
at fixing it.

-apw

Re: checkpatch false positive

[Joe Perches]

On Wed, 2010-03-17 at 11:00 +0000, Richard Kennedy wrote:
> I'm getting this error from checkpatch which is a false positive AFAICT.
> I don't see any other way to code this macro so maybe this rule
> shouldn't apply?.
> 
> 	ERROR: space prohibited before open square bracket '['
> 	#24: FILE: drivers/staging/wlan-ng/p80211wext.c:1685:
> 	+#define IW_IOCTL(x) [(x)-SIOCSIWCOMMIT]

While true that this is a false positive, hiding array indexing
brackets in a macro doesn't seem a good idea.

Maybe it'd be better to move the brackets to the use?

Re: checkpatch false positive

[Richard Kennedy]

On Wed, 2010-03-17 at 08:25 -0700, Joe Perches wrote:
> On Wed, 2010-03-17 at 11:00 +0000, Richard Kennedy wrote:
> > I'm getting this error from checkpatch which is a false positive AFAICT.
> > I don't see any other way to code this macro so maybe this rule
> > shouldn't apply?.
> > 
> > 	ERROR: space prohibited before open square bracket '['
> > 	#24: FILE: drivers/staging/wlan-ng/p80211wext.c:1685:
> > 	+#define IW_IOCTL(x) [(x)-SIOCSIWCOMMIT]
> 
> While true that this is a false positive, hiding array indexing
> brackets in a macro doesn't seem a good idea.
> 
> Maybe it'd be better to move the brackets to the use?
> 
> 
err maybe ;)

I copied it from driver/net/wireless/ipw2x00/ipw2200.c 

It just reduces typing when initialising the array :-

	#define IW_IOCTL(x) [(x)-SIOCSIWCOMMIT]
	static iw_handler p80211wext_handlers[] = {
		IW_IOCTL(SIOCSIWCOMMIT) = (iw_handler) p80211wext_siwcommit,
		...

Oh, having quickly looked at wireless.h, I see there is already a macro
IW_IOCTL_IDX so I guess I should have used that!

would something like this be better?

	static iw_handler p80211wext_handlers[] = {
		[IW_IOCTL_IDX(SIOCSIWCOMMIT)] = (iw_handler) p80211wext_siwcommit,
		...

regards
Richard