checkpatch on Kconfig files

checkpatch on Kconfig files

[Jean Delvare]

Hi Andy, hi Joe,

When running checkpatch on a patch which tweaks many Kconfig entries, I
got the following output:

WARNING: please write a paragraph that describes the config symbol fully
#74: FILE: drivers/hid/Kconfig:142:
 config HID_BELKIN

WARNING: please write a paragraph that describes the config symbol fully
#82: FILE: drivers/hid/Kconfig:149:
 config HID_CHERRY

WARNING: please write a paragraph that describes the config symbol fully
#135: FILE: drivers/hid/Kconfig:415:
 config HID_MICROSOFT

total: 0 errors, 3 warnings, 88 lines checked

I am not adding these entries, just tweaking the dependencies. Thus I
don't think it makes sense to print these warnings, they aren't
relevant to the changes I am making. There comment in checkpatch says:

# check for Kconfig help text having a real description
# Only applies when adding the entry originally, after that we do not have
# sufficient context to determine whether it is indeed long enough.

So I suspect these warnings aren't supposed to be displayed. I recall
hitting this many times in the past, in fact I think this is the most
frequent and oldest false positive I get from checkpatch. So I would
appreciate if this could be fixed, either by really limiting the
warning to Kconfig entries being added (if you can) or by dropping the
check altogether (if you can't.)

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: checkpatch on Kconfig files

[Joe Perches]

(adding Andi Kleen)

On Fri, 2014-03-21 at 09:42 +0100, Jean Delvare wrote:
> Hi Andy, hi Joe,
> 
> When running checkpatch on a patch which tweaks many Kconfig entries, I
> got the following output:
> 
> WARNING: please write a paragraph that describes the config symbol fully
> #74: FILE: drivers/hid/Kconfig:142:
>  config HID_BELKIN
> 
> WARNING: please write a paragraph that describes the config symbol fully
> #82: FILE: drivers/hid/Kconfig:149:
>  config HID_CHERRY
> 
> WARNING: please write a paragraph that describes the config symbol fully
> #135: FILE: drivers/hid/Kconfig:415:
>  config HID_MICROSOFT
> 
> total: 0 errors, 3 warnings, 88 lines checked
> 
> I am not adding these entries, just tweaking the dependencies. Thus I
> don't think it makes sense to print these warnings, they aren't
> relevant to the changes I am making. There comment in checkpatch says:
> 
> # check for Kconfig help text having a real description
> # Only applies when adding the entry originally, after that we do not have
> # sufficient context to determine whether it is indeed long enough.
> 
> So I suspect these warnings aren't supposed to be displayed. I recall
> hitting this many times in the past, in fact I think this is the most
> frequent and oldest false positive I get from checkpatch. So I would
> appreciate if this could be fixed, either by really limiting the
> warning to Kconfig entries being added (if you can) or by dropping the
> check altogether (if you can't.)

I believe that was Andi Kleen's pet peeve,
so I'll punt it back to him.

Re: checkpatch on Kconfig files

[Andi Kleen]

> I believe that was Andi Kleen's pet peeve,
> so I'll punt it back to him.

This was always in checkpatch. If you touch/move some existing
code it blames you for the issues that were already there.

Not specific to this check.

If you disagree with the warnings just ignore them.

-Andi

Re: checkpatch on Kconfig files

[Jean Delvare]

Hi Andi,

On Fri, 21 Mar 2014 14:21:21 +0100, Andi Kleen wrote:
> > I believe that was Andi Kleen's pet peeve,
> > so I'll punt it back to him.
> 
> This was always in checkpatch. If you touch/move some existing
> code it blames you for the issues that were already there.
> 
> Not specific to this check.

Actually it is. I was not moving the Kconfig entry, and I was not
touching its help text either. Here checkpatch complained about
something which was in the context of the patch, not in added or
modified lines. I am not aware of any other check doing that,
thankfully.

If I obeyed to checkpatch and "fixed" the help text to make it happy,
the reviewer would have (rightfully) rejected my patch as mixing
unrelated changes together.

> If you disagree with the warnings just ignore them.

This is a false positive, and checkpatch explicitly asks for these to
be reported. Which is what I'm doing. Better tools make future
contributions better and easier.

-- 
Jean Delvare
SUSE L3 Support

Re: checkpatch on Kconfig files

[Joe Perches]

On Fri, 2014-03-21 at 14:36 +0100, Jean Delvare wrote:
> Hi Andi,
> 
> On Fri, 21 Mar 2014 14:21:21 +0100, Andi Kleen wrote:
> > > I believe that was Andi Kleen's pet peeve,
> > > so I'll punt it back to him.
> > 
> > This was always in checkpatch. If you touch/move some existing
> > code it blames you for the issues that were already there.
> > 
> > Not specific to this check.
> 
> Actually it is. I was not moving the Kconfig entry, and I was not
> touching its help text either. Here checkpatch complained about
> something which was in the context of the patch, not in added or
> modified lines. I am not aware of any other check doing that,
> thankfully.

I think there's one other --strict test for alignment
that uses context lines to bleat a message

change something like:

-	printk(KERN_WARNING "long line %d\n",
 	       length);
to
+	pr_warn("long line %d\n",
 	       length);

changing just the printk -> pr_warn line but not the
length line below it and checkpatch will bleat a
"bad alignment" message.

> This is a false positive, and checkpatch explicitly asks for these to
> be reported. Which is what I'm doing. Better tools make future
> contributions better and easier.

Another option would be to change the message when
it's in context lines and not in added lines to
something other than a demand for help text.

Or make it a --strict only CHK message when it's in
context and not added as part of the submission.

Re: checkpatch on Kconfig files

[Guenter Roeck]

On Fri, Mar 21, 2014 at 09:42:39AM +0100, Jean Delvare wrote:
> Hi Andy, hi Joe,
> 
> When running checkpatch on a patch which tweaks many Kconfig entries, I
> got the following output:
> 
> WARNING: please write a paragraph that describes the config symbol fully
> #74: FILE: drivers/hid/Kconfig:142:
>  config HID_BELKIN
> 
> WARNING: please write a paragraph that describes the config symbol fully
> #82: FILE: drivers/hid/Kconfig:149:
>  config HID_CHERRY
> 
> WARNING: please write a paragraph that describes the config symbol fully
> #135: FILE: drivers/hid/Kconfig:415:
>  config HID_MICROSOFT
> 
> total: 0 errors, 3 warnings, 88 lines checked
> 
> I am not adding these entries, just tweaking the dependencies. Thus I
> don't think it makes sense to print these warnings, they aren't
> relevant to the changes I am making. There comment in checkpatch says:
> 
> # check for Kconfig help text having a real description
> # Only applies when adding the entry originally, after that we do not have
> # sufficient context to determine whether it is indeed long enough.
> 
> So I suspect these warnings aren't supposed to be displayed. I recall
> hitting this many times in the past, in fact I think this is the most
> frequent and oldest false positive I get from checkpatch. So I would
> appreciate if this could be fixed, either by really limiting the
> warning to Kconfig entries being added (if you can) or by dropping the
> check altogether (if you can't.)
> 
For my part this is the one and only checkpatch warning which I don't
enfore and happily ignore myself. Might be interesting to get feedback
from others if they think it is useful or just annoying.

Guenter