linux-kernel - Re: Checkpatch false positive?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20090128093525.GB10291@shadowen.org>
Date:	Wed, 28 Jan 2009 09:35:25 +0000
From:	Andy Whitcroft <apw@...onical.com>
To:	Adrian Bunk <bunk@...nel.org>
Cc:	Jan Kara <jack@...e.cz>, linux-kernel@...r.kernel.org
Subject: Re: Checkpatch false positive?

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/