Checkpatch false positive?

Checkpatch false positive?

[Jan Kara]

  Hi,

  I've used checkpatch.pl to verify one of my patches. It complains:

ERROR: trailing statements should be on next line
#167: FILE: fs/quota/quota_tree.c:249:
+       for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
[...]
             i++, ddquot += info->dqi_entry_size);

  But the code looks like:
        for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
             i < qtree_dqstr_in_blk(info) && !qtree_entry_unused(info, ddquot);
             i++, ddquot += info->dqi_entry_size);

  Which is IMHO correct. Maybe it's because the for has actually empty body
and the ; is at the end of the line with for. But I didn't find anything in
CodingStyle that would forbid
	for (...);
and
	for (...)
		;
Looks a bit strange.

									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: Checkpatch false positive?

[Adrian Bunk]

On Tue, Jan 27, 2009 at 04:49:05PM +0100, Jan Kara wrote:
>   Hi,
> 
>   I've used checkpatch.pl to verify one of my patches. It complains:
> 
> ERROR: trailing statements should be on next line
> #167: FILE: fs/quota/quota_tree.c:249:
> +       for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
> [...]
>              i++, ddquot += info->dqi_entry_size);
> 
>   But the code looks like:
>         for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
>              i < qtree_dqstr_in_blk(info) && !qtree_entry_unused(info, ddquot);
>              i++, ddquot += info->dqi_entry_size);
> 
>   Which is IMHO correct. Maybe it's because the for has actually empty body
> and the ; is at the end of the line with for. But I didn't find anything in
> CodingStyle that would forbid
> 	for (...);
> and
> 	for (...)
> 		;
> Looks a bit strange.

for (...); is a common C programming error, usually it's some kind of:

	for(........);
		do_something();

This code does something different than intended.
And yes, we had such bugs in the kernel.


	for(........)
		;

is correct. The "looks a bit strange" is what actually tells readers 
what the code is doing (and that the author did it intentionally).


> 									Honza

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: Checkpatch false positive?

[Jan Kara]

On Tue 27-01-09 18:06:01, Adrian Bunk wrote:
> On Tue, Jan 27, 2009 at 04:49:05PM +0100, Jan Kara wrote:
> >   Hi,
> > 
> >   I've used checkpatch.pl to verify one of my patches. It complains:
> > 
> > ERROR: trailing statements should be on next line
> > #167: FILE: fs/quota/quota_tree.c:249:
> > +       for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
> > [...]
> >              i++, ddquot += info->dqi_entry_size);
> > 
> >   But the code looks like:
> >         for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
> >              i < qtree_dqstr_in_blk(info) && !qtree_entry_unused(info, ddquot);
> >              i++, ddquot += info->dqi_entry_size);
> > 
> >   Which is IMHO correct. Maybe it's because the for has actually empty body
> > and the ; is at the end of the line with for. But I didn't find anything in
> > CodingStyle that would forbid
> > 	for (...);
> > and
> > 	for (...)
> > 		;
> > Looks a bit strange.
> 
> for (...); is a common C programming error, usually it's some kind of:
> 
> 	for(........);
> 		do_something();
> 
> This code does something different than intended.
> And yes, we had such bugs in the kernel.
> 
> 
> 	for(........)
> 		;
> 
> is correct. The "looks a bit strange" is what actually tells readers 
> what the code is doing (and that the author did it intentionally).
  OK, makes some sence. Thanks for explanation.

									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: Checkpatch false positive?

[Andy Whitcroft]

On Tue, Jan 27, 2009 at 06:06:01PM +0200, Adrian Bunk wrote:
> On Tue, Jan 27, 2009 at 04:49:05PM +0100, Jan Kara wrote:
> >   Hi,
> > 
> >   I've used checkpatch.pl to verify one of my patches. It complains:
> > 
> > ERROR: trailing statements should be on next line
> > #167: FILE: fs/quota/quota_tree.c:249:
> > +       for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
> > [...]
> >              i++, ddquot += info->dqi_entry_size);
> > 
> >   But the code looks like:
> >         for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
> >              i < qtree_dqstr_in_blk(info) && !qtree_entry_unused(info, ddquot);
> >              i++, ddquot += info->dqi_entry_size);
> > 
> >   Which is IMHO correct. Maybe it's because the for has actually empty body
> > and the ; is at the end of the line with for. But I didn't find anything in
> > CodingStyle that would forbid
> > 	for (...);
> > and
> > 	for (...)
> > 		;
> > Looks a bit strange.
> 
> for (...); is a common C programming error, usually it's some kind of:
> 
> 	for(........);
> 		do_something();
> 
> This code does something different than intended.
> And yes, we had such bugs in the kernel.
> 
> 
> 	for(........)
> 		;
> 
> is correct. The "looks a bit strange" is what actually tells readers 
> what the code is doing (and that the author did it intentionally).

Yeah its about being explicit that you intended there to be an empty
statement in this construct.  We tend to get bitten even more by if form
of this:

	if (foo);
		something();

But we catch them all.  And always remember if you really think it looks
better or makes more sense one way and checkpatch is upset you can
ignore checkpatch.  It is advice on what will be accepted not
necessarily the final arbiter.  That is the maintainers role, checkpatch
is a tool to help you.

-apw

Re: Checkpatch false positive?

[Arjan van de Ven]

On Tue, 27 Jan 2009 16:49:05 +0100
Jan Kara <jack@suse.cz> wrote:

>   Hi,
> 
>   I've used checkpatch.pl to verify one of my patches. It complains:
> 
> ERROR: trailing statements should be on next line
> #167: FILE: fs/quota/quota_tree.c:249:
> +       for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
> [...]
>              i++, ddquot += info->dqi_entry_size);
> 
>   But the code looks like:
>         for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
>              i < qtree_dqstr_in_blk(info)
> && !qtree_entry_unused(info, ddquot); i++, ddquot +=
> info->dqi_entry_size);
> 

while tihs might be correct C... don't you think it would be much
better to actually have a statement here rather than cramming
everything into the for ?

Re: Checkpatch false positive?

[Jan Kara]

On Tue 27-01-09 08:19:54, Arjan van de Ven wrote:
> On Tue, 27 Jan 2009 16:49:05 +0100
> Jan Kara <jack@suse.cz> wrote:
> 
> >   Hi,
> > 
> >   I've used checkpatch.pl to verify one of my patches. It complains:
> > 
> > ERROR: trailing statements should be on next line
> > #167: FILE: fs/quota/quota_tree.c:249:
> > +       for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
> > [...]
> >              i++, ddquot += info->dqi_entry_size);
> > 
> >   But the code looks like:
> >         for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
> >              i < qtree_dqstr_in_blk(info)
> > && !qtree_entry_unused(info, ddquot); i++, ddquot +=
> > info->dqi_entry_size);
> > 
> 
> while tihs might be correct C... don't you think it would be much
> better to actually have a statement here rather than cramming
> everything into the for ?
  This is an old code and I was just wrapping lines to fit into 80 chars...
But you're right, I can rewrite the loop into more readable form when I'm
at it.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Checkpatch false positive?

[Matthew Helsley]

Hello checkpatch.pl maintainers,

	I'm adding a new thread flag and I get this apparent checkpatch false
positive:


ERROR: do not modify files in include/asm, change architecture specific files in include/asm-<architecture>
#36: +++ linux-2.6.27-rc1-mm1/arch/sparc/include/asm/thread_info_32.h

ERROR: do not modify files in include/asm, change architecture specific files in include/asm-<architecture>
#63: +++ linux-2.6.27-rc1-mm1/arch/sparc/include/asm/thread_info_64.h

ERROR: do not modify files in include/asm, change architecture specific files in include/asm-<architecture>
#289: +++ linux-2.6.27-rc1-mm1/arch/parisc/include/asm/thread_info.h

patches/0001-Container-Freezer-Add-TIF_FREEZE-flag-to-all-archit.patch total: 3 errors, 0 warnings, 214 lines checked

patches/0001-Container-Freezer-Add-TIF_FREEZE-flag-to-all-archit.patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.


	Which happens for every arch where the file has been moved under the
arch/ directory (sparc and parisc so far). Should this check for
arch/<foo>/include/asm before giving an ERROR? Should
arch/<foo>/include/asm only trigger a WARNING in this case?

Cheers,
	-Matt Helsley

Re: Checkpatch false positive?

[Andy Whitcroft]

On Thu, Jul 31, 2008 at 09:51:02PM -0700, Matthew Helsley wrote:
> Hello checkpatch.pl maintainers,
> 
> 	I'm adding a new thread flag and I get this apparent checkpatch false
> positive:
> 
> 
> ERROR: do not modify files in include/asm, change architecture specific files in include/asm-<architecture>
> #36: +++ linux-2.6.27-rc1-mm1/arch/sparc/include/asm/thread_info_32.h
> 
> ERROR: do not modify files in include/asm, change architecture specific files in include/asm-<architecture>
> #63: +++ linux-2.6.27-rc1-mm1/arch/sparc/include/asm/thread_info_64.h
> 
> ERROR: do not modify files in include/asm, change architecture specific files in include/asm-<architecture>
> #289: +++ linux-2.6.27-rc1-mm1/arch/parisc/include/asm/thread_info.h
> 
> patches/0001-Container-Freezer-Add-TIF_FREEZE-flag-to-all-archit.patch total: 3 errors, 0 warnings, 214 lines checked
> 
> patches/0001-Container-Freezer-Add-TIF_FREEZE-flag-to-all-archit.patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 
> 	Which happens for every arch where the file has been moved under the
> arch/ directory (sparc and parisc so far). Should this check for
> arch/<foo>/include/asm before giving an ERROR? Should
> arch/<foo>/include/asm only trigger a WARNING in this case?
> 
> Cheers,
> 	-Matt Helsley

Yes, that check isn't very well anchored.  I've tightened that up in the
latest version.  Could you check your patch with the latest testing
version for me:

    http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-testing

-apw

Re: Checkpatch false positive?

[Matt Helsley]

On Sun, 2008-08-03 at 23:18 +0100, Andy Whitcroft wrote:
> On Thu, Jul 31, 2008 at 09:51:02PM -0700, Matthew Helsley wrote:
> > Hello checkpatch.pl maintainers,
> > 
> > 	I'm adding a new thread flag and I get this apparent checkpatch false
> > positive:
> > 
> > 
> > ERROR: do not modify files in include/asm, change architecture specific files in include/asm-<architecture>
> > #36: +++ linux-2.6.27-rc1-mm1/arch/sparc/include/asm/thread_info_32.h
> > 
> > ERROR: do not modify files in include/asm, change architecture specific files in include/asm-<architecture>
> > #63: +++ linux-2.6.27-rc1-mm1/arch/sparc/include/asm/thread_info_64.h
> > 
> > ERROR: do not modify files in include/asm, change architecture specific files in include/asm-<architecture>
> > #289: +++ linux-2.6.27-rc1-mm1/arch/parisc/include/asm/thread_info.h
> > 
> > patches/0001-Container-Freezer-Add-TIF_FREEZE-flag-to-all-archit.patch total: 3 errors, 0 warnings, 214 lines checked
> > 
> > patches/0001-Container-Freezer-Add-TIF_FREEZE-flag-to-all-archit.patch has style problems, please review.  If any of these errors
> > are false positives report them to the maintainer, see
> > CHECKPATCH in MAINTAINERS.
> > 
> > 
> > 	Which happens for every arch where the file has been moved under the
> > arch/ directory (sparc and parisc so far). Should this check for
> > arch/<foo>/include/asm before giving an ERROR? Should
> > arch/<foo>/include/asm only trigger a WARNING in this case?
> > 
> > Cheers,
> > 	-Matt Helsley
> 
> Yes, that check isn't very well anchored.  I've tightened that up in the
> latest version.  Could you check your patch with the latest testing
> version for me:
> 
>     http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-testing
> 
> -apw

Output looks good:

patches/0001-Container-Freezer-Add-TIF_FREEZE-flag-to-all-archit.patch
total: 0 errors, 0 warnings, 152 lines checked

patches/0001-Container-Freezer-Add-TIF_FREEZE-flag-to-all-archit.patch
has no obvious style problems and is ready for submission.


Thanks!

Cheers,
	-Matt Helsley